Re: [OMPI devel] cannot push directly to master anymore

2018-01-31 Thread Gilles Gouaillardet 
It seems we are all on the same page, so I am fine to keep things as is too.

IIRC, GitHub.com does not allow any *user* pre-commit hooks for
security reasons.

"Auto-merge when all CI completes" would be a nice feature to have,
but I am not sure this
is something we want to develop and maintain.


Cheers,

Gilles

On Thu, Feb 1, 2018 at 3:44 AM, Barrett, Brian via devel
 wrote:
>
>> On Jan 31, 2018, at 8:33 AM, r...@open-mpi.org wrote:
>>
>>
>>
>>> On Jan 31, 2018, at 7:36 AM, Jeff Squyres (jsquyres)  
>>> wrote:
>>>
>>> On Jan 31, 2018, at 10:14 AM, Gilles Gouaillardet 
>>>  wrote:

 I tried to push some trivial commits directly to the master branch and
 was surprised that is no more allowed.

 The error message is not crystal clear, but I guess the root cause is
 the two newly required checks (Commit email checker and
 Signed-off-by-checker) were not performed.
>>>
>>> That is probably my fault; I was testing something and didn't mean to leave 
>>> that enabled.  Oops -- sorry.  :-(
>>>
>>> That being said -- is it a terrible thing to require a PR to ensure that we 
>>> get a valid email address (e.g., not a "root@localhost") and that we have a 
>>> proper signed-off-by line?
>>
>>>
 /* note if the commit is trivial, then it is possible to add the following 
 line
 [skip ci]
 into the commit message, so Jenkins will not check the PR. */
>>>
>>> We've had some discussions about this on the Tuesday calls -- the point was 
>>> made that if you allow skipping CI for "trivial" commits, it starts you 
>>> down the slippery slope of precisely defining what "trivial" means.  
>>> Indeed, I know that I have been guilty of making a "trivial" change that 
>>> ended up breaking something.
>>>
>>> FWIW, I have stopped using the "[skip ci]" stuff -- even if I made 
>>> docs-only changes.  I.e., just *always* go through CI.  That way there's 
>>> never any question, and never any possibility of a human mistake (e.g., 
>>> accidentally marking "[skip ci]" on a PR that really should have had CI).
>>
>> If CI takes 30 min, then not a problem - when CI takes 6 hours (as it 
>> sometimes does), then that’s a different story.
>
> If CI takes more than about 30 minutes, something’s broke.  Unfortunately, 
> Jenkins makes that particular problem hard to deal with (monitoring for job 
> length).  I have some thoughts on how to make it better for the OMPI Jenkins, 
> but am short on implementation time.  So if we have any volunteers to help 
> here… :)
>
> I have no objections to PR-only for master.  The other option is pre-commit 
> hooks, but that’s kind of ugly.  We allow “rebase and merge” if people don’t 
> like the merge commits on trunk.  Also makes updating NEWS/README items 
> easier when we eventually get that workflow built (because you can change the 
> message later with a PR, but not a commit).
>
> We can also experiment with adding GitHub messages to the PR when CI 
> completes, if people would like an async notification...
>
> Brian
> ___
> devel mailing list
> devel@lists.open-mpi.org
> https://lists.open-mpi.org/mailman/listinfo/devel
___
devel mailing list
devel@lists.open-mpi.org
https://lists.open-mpi.org/mailman/listinfo/devel

[OMPI devel] hwloc issues in this week telcon?

2018-01-31 Thread Brice Goglin 
Hello

Two hwloc issues are listed in this week telcon:

"hwloc2 WIP, may need help with."
https://github.com/open-mpi/ompi/pull/4677
* Is this really a 3.0.1 thing? I thought hwloc2 was only for 3.1+
* As I replied in this PR, I have some patches but I need help for
testing them. Can you list some good test cases?

"Issue - hwloc can't handle cuda from a different location"
I have no idea what this is about. Is there a github issue for this?

Thanks
Brice

___
devel mailing list
devel@lists.open-mpi.org
https://lists.open-mpi.org/mailman/listinfo/devel

Re: [OMPI devel] cannot push directly to master anymore

2018-01-31 Thread Barrett, Brian via devel 

> On Jan 31, 2018, at 8:33 AM, r...@open-mpi.org wrote:
> 
> 
> 
>> On Jan 31, 2018, at 7:36 AM, Jeff Squyres (jsquyres)  
>> wrote:
>> 
>> On Jan 31, 2018, at 10:14 AM, Gilles Gouaillardet 
>>  wrote:
>>> 
>>> I tried to push some trivial commits directly to the master branch and
>>> was surprised that is no more allowed.
>>> 
>>> The error message is not crystal clear, but I guess the root cause is
>>> the two newly required checks (Commit email checker and
>>> Signed-off-by-checker) were not performed.
>> 
>> That is probably my fault; I was testing something and didn't mean to leave 
>> that enabled.  Oops -- sorry.  :-(
>> 
>> That being said -- is it a terrible thing to require a PR to ensure that we 
>> get a valid email address (e.g., not a "root@localhost") and that we have a 
>> proper signed-off-by line?
> 
>> 
>>> /* note if the commit is trivial, then it is possible to add the following 
>>> line
>>> [skip ci]
>>> into the commit message, so Jenkins will not check the PR. */
>> 
>> We've had some discussions about this on the Tuesday calls -- the point was 
>> made that if you allow skipping CI for "trivial" commits, it starts you down 
>> the slippery slope of precisely defining what "trivial" means.  Indeed, I 
>> know that I have been guilty of making a "trivial" change that ended up 
>> breaking something.
>> 
>> FWIW, I have stopped using the "[skip ci]" stuff -- even if I made docs-only 
>> changes.  I.e., just *always* go through CI.  That way there's never any 
>> question, and never any possibility of a human mistake (e.g., accidentally 
>> marking "[skip ci]" on a PR that really should have had CI).
> 
> If CI takes 30 min, then not a problem - when CI takes 6 hours (as it 
> sometimes does), then that’s a different story.

If CI takes more than about 30 minutes, something’s broke.  Unfortunately, 
Jenkins makes that particular problem hard to deal with (monitoring for job 
length).  I have some thoughts on how to make it better for the OMPI Jenkins, 
but am short on implementation time.  So if we have any volunteers to help 
here… :)

I have no objections to PR-only for master.  The other option is pre-commit 
hooks, but that’s kind of ugly.  We allow “rebase and merge” if people don’t 
like the merge commits on trunk.  Also makes updating NEWS/README items easier 
when we eventually get that workflow built (because you can change the message 
later with a PR, but not a commit).

We can also experiment with adding GitHub messages to the PR when CI completes, 
if people would like an async notification...

Brian
___
devel mailing list
devel@lists.open-mpi.org
https://lists.open-mpi.org/mailman/listinfo/devel

Re: [OMPI devel] cannot push directly to master anymore

2018-01-31 Thread r...@open-mpi.org 


> On Jan 31, 2018, at 8:41 AM, Jeff Squyres (jsquyres)  
> wrote:
> 
> On Jan 31, 2018, at 11:33 AM, r...@open-mpi.org wrote:
>> 
>> If CI takes 30 min, then not a problem - when CI takes 6 hours (as it 
>> sometimes does), then that’s a different story.
> 
> Fair point; that's why I experimented with (and accidentally left enabled) 
> only having the 2 pretty-much-immediate CI checks (email checker and 
> signed-off-by checker).
> 
> We have definitely seen unreliable CI hang for hours (or days... or even get 
> abandoned when a CI server is reset).  So it's understandable that sometimes 
> people merge before waiting for CI to complete.
> 
> But I think the central question here is: do we want to leave it set as it is 
> right now:
> 
> 1. you *must* make a PR
> 2. the email-checker and signed-off-by-checker CI *must* pass on that PR
> 
> This still allows you to merge early (i.e., before other CI completes).  
> That's a different issue, and is probably ok the way that it is currently 
> handled (i.e., individual developer's discretion -- usually let all the CI 
> finish, but merge early when the situation warrants it).

I personally have no objections

> 
> -- 
> Jeff Squyres
> jsquy...@cisco.com
> 
> 
> 
> ___
> devel mailing list
> devel@lists.open-mpi.org
> https://lists.open-mpi.org/mailman/listinfo/devel

___
devel mailing list
devel@lists.open-mpi.org
https://lists.open-mpi.org/mailman/listinfo/devel

Re: [OMPI devel] cannot push directly to master anymore

2018-01-31 Thread Jeff Squyres (jsquyres) 
On Jan 31, 2018, at 11:33 AM, r...@open-mpi.org wrote:
> 
> If CI takes 30 min, then not a problem - when CI takes 6 hours (as it 
> sometimes does), then that’s a different story.

Fair point; that's why I experimented with (and accidentally left enabled) only 
having the 2 pretty-much-immediate CI checks (email checker and signed-off-by 
checker).

We have definitely seen unreliable CI hang for hours (or days... or even get 
abandoned when a CI server is reset).  So it's understandable that sometimes 
people merge before waiting for CI to complete.

But I think the central question here is: do we want to leave it set as it is 
right now:

1. you *must* make a PR
2. the email-checker and signed-off-by-checker CI *must* pass on that PR

This still allows you to merge early (i.e., before other CI completes).  That's 
a different issue, and is probably ok the way that it is currently handled 
(i.e., individual developer's discretion -- usually let all the CI finish, but 
merge early when the situation warrants it).

-- 
Jeff Squyres
jsquy...@cisco.com



___
devel mailing list
devel@lists.open-mpi.org
https://lists.open-mpi.org/mailman/listinfo/devel

Re: [OMPI devel] cannot push directly to master anymore

2018-01-31 Thread r...@open-mpi.org 


> On Jan 31, 2018, at 7:36 AM, Jeff Squyres (jsquyres)  
> wrote:
> 
> On Jan 31, 2018, at 10:14 AM, Gilles Gouaillardet 
>  wrote:
>> 
>> I tried to push some trivial commits directly to the master branch and
>> was surprised that is no more allowed.
>> 
>> The error message is not crystal clear, but I guess the root cause is
>> the two newly required checks (Commit email checker and
>> Signed-off-by-checker) were not performed.
> 
> That is probably my fault; I was testing something and didn't mean to leave 
> that enabled.  Oops -- sorry.  :-(
> 
> That being said -- is it a terrible thing to require a PR to ensure that we 
> get a valid email address (e.g., not a "root@localhost") and that we have a 
> proper signed-off-by line?

> 
>> /* note if the commit is trivial, then it is possible to add the following 
>> line
>> [skip ci]
>> into the commit message, so Jenkins will not check the PR. */
> 
> We've had some discussions about this on the Tuesday calls -- the point was 
> made that if you allow skipping CI for "trivial" commits, it starts you down 
> the slippery slope of precisely defining what "trivial" means.  Indeed, I 
> know that I have been guilty of making a "trivial" change that ended up 
> breaking something.
> 
> FWIW, I have stopped using the "[skip ci]" stuff -- even if I made docs-only 
> changes.  I.e., just *always* go through CI.  That way there's never any 
> question, and never any possibility of a human mistake (e.g., accidentally 
> marking "[skip ci]" on a PR that really should have had CI).

If CI takes 30 min, then not a problem - when CI takes 6 hours (as it sometimes 
does), then that’s a different story.


> 
> -- 
> Jeff Squyres
> jsquy...@cisco.com
> 
> 
> 
> ___
> devel mailing list
> devel@lists.open-mpi.org
> https://lists.open-mpi.org/mailman/listinfo/devel

___
devel mailing list
devel@lists.open-mpi.org
https://lists.open-mpi.org/mailman/listinfo/devel

[OMPI devel] cannot push directly to master anymore

2018-01-31 Thread Gilles Gouaillardet 
Folks,

I tried to push some trivial commits directly to the master branch and
was surprised that is no more allowed.

The error message is not crystal clear, but I guess the root cause is
the two newly required checks (Commit email checker and
Signed-off-by-checker) were not performed.

As a kind of workaround, I created a PR so these two checks can be
performed and the commits marked as good, and then fast forwarded my
master branch and pushed it into master.

I do not think these checks can be performed "on the fly" by the
GitHub servers, so if they are marked as required, it means it is no
more possible to push directly to master.

Is this a behavior we expected ? If not, are we fine with it ?
Generally speaking, should we always issue PR and use the GitHub merge button ?

/* note if the commit is trivial, then it is possible to add the following line
[skip ci]
into the commit message, so Jenkins will not check the PR. */


Cheers,

Gilles
___
devel mailing list
devel@lists.open-mpi.org
https://lists.open-mpi.org/mailman/listinfo/devel