Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

[Simon Marchi <simon.marchi@ericsson.com>]

On 2018-10-29 12:42 p.m., Sergio Durigan Junior wrote:
> On Monday, October 29 2018, John Darrington wrote:
> 
>> On Mon, Oct 29, 2018 at 03:51:55PM +0000, Simon Marchi wrote:
>>      On 2018-10-29 5:11 a.m., Rainer Orth wrote:
>>      > Hi John,
>>      > 
>>      >> However I've checked in a fix for this issue, and tested it by building
>>      >> natively with a hacked set of standard include headers.
>>      > 
>>      > you always need to post patches here, if only for reference.
>>      
>> Doesn't that make the gdb-cvs list completely redundant?

Perhaps, I don't know.  Personally, I am not subscribed to gdb-cvs.  And generally,
when posting on gdb-patches, you'll mention that the patch was pushed as "obvious",
which the commit message won't necessarily contain.

It's also sometimes necessary to discuss a patch that was initially committed as
obvious.  The post on gdb-patches would be the place for that.

>>      Not only should you post here the patches you push as obvious, but I don't
>>      think that this:
>>      
>>      https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=98a17ece013cb94cd602496b9efb92b8816b3953
>>      
>>      falls under the obvious rule:
>>
>> But a number of people had complained that their build was broken, and
>> this was a fix for that.   I judged that in consideration of those
>> people fixding their problem was more important than strict observance 
>> of protocol.

Let me reassure you, I completely understand that the intention was good.  I don't
want you to feel bad in any way for trying to fix things up!  Next time, post the
patch to the mailing list for review.  If the patch title contains "Fix build for...",
it is likely that it will be reviewed quite quickly.

>>      
>>      I can't judge whether the patch is right or not with a quick glance, but it
>>      certainly is complex enough to warrant a discussion (as Rainer's reply below
>>      shows).
>>
>>      
>>      Additionally, it seems like the initial 4-patch series was pushed without
>>      explicit approval from a maintainer (at least I can't find any).  Next time,
>>      please wait to have an approval before pushing.  If you are not sure whether
>>      a reply constitutes an a approval, it's better to ask the maintainer to
>>      clarify.
>>
>> All of those patches were certainly discussed.   In the past, when I've
>> followed up a person who has commented on a patch, but been vague about
>> approval, I have had either a piqued response; or no response at all.
> 
> We were certainly discussing the patches, but they were not approved by
> anyone.  It's also worth mentioning that I raised various points that
> were not addressed (even though we discussed them).  It is still a
> requirement that the patches need to be approved by at least one
> maintainer before it is pushed to the repository.

Yep.

>> If you think it necesary however I can revert anything you think hasn't
>> had enough discussion.
> 
> Yes, please.  The patch series is not ready to be pushed yet; there are
> a bunch of points I raised that were not addressed (and are now causing
> the failures).  I am not a global maintainer, but it is my opinion that
> the patch series should be reverted for now.  We can continue discussing
> and fixing things with it here in the list.

I have reverted the initial series as well as the fixup.  Sergio has mentioned on IRC
that it causes some test failures on the buildbot that cause the test time very long.
So this needs more testing before we push it again.

Simon