RFC: split OBJS lines to one object per line

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC: split OBJS lines to one object per line

Andres Freund
Hi,

one of the most frequent conflicts I see is that two patches add files
to OBJS (or one of its other spellings), and there are conflicts because
another file has been added.

Right now there's two reasons why that's likely to happen:
1) By listing multiple objects for each line, we get a conflict whenever
   one of the other files on that lines gets modified
2) Due to our line-length conventions, we have to re-flow long lines,
   which often triggers reflowing subsequent lines too.

Now, obviously these types of conflicts are easy enough to resolve, but
it's still annoying.  It seems that this would be substantially less
often a problem if we just split such lines to one file per
line. E.g. instead of

OBJS_COMMON = base64.o config_info.o controldata_utils.o d2s.o exec.o f2s.o \
        file_perm.o ip.o keywords.o kwlookup.o link-canary.o md5.o \
        pg_lzcompress.o pgfnames.o psprintf.o relpath.o \
        rmtree.o saslprep.o scram-common.o string.o  stringinfo.o \
        unicode_norm.o username.o wait_error.o

have

OBJS_COMMON = \
        base64.o \
        config_info.o \
        controldata_utils.o \
        d2s.o \
        exec.o \
        f2s.o \
        file_perm.o \
        ip.o \
        keywords.o \
        kwlookup.o \
        link-canary.o \
        md5.o \
        pg_lzcompress.o \
        pgfnames.o \
        psprintf.o \
        relpath.o \
        rmtree.o \
        saslprep.o \
        scram-common.o \
        string.o \
        stringinfo.o \
        unicode_norm.o \
        username.o \
        wait_error.o

a one-off conversion of this seems easy enough to script.

Comments?

- Andres


Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Peter Geoghegan-4
On Tue, Oct 29, 2019 at 1:09 PM Andres Freund <[hidden email]> wrote:
> Comments?

I think that this is a good idea. I see no downside.


--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Tom Lane-2
In reply to this post by Andres Freund
Andres Freund <[hidden email]> writes:
> one of the most frequent conflicts I see is that two patches add files
> to OBJS (or one of its other spellings), and there are conflicts because
> another file has been added.
> ...
> Now, obviously these types of conflicts are easy enough to resolve, but
> it's still annoying.  It seems that this would be substantially less
> often a problem if we just split such lines to one file per
> line.

We did something similar not too long ago in configure.in (bfa6c5a0c),
and it seems to have helped.  +1

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Andres Freund
Hi,

On 2019-10-29 16:31:11 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > one of the most frequent conflicts I see is that two patches add files
> > to OBJS (or one of its other spellings), and there are conflicts because
> > another file has been added.
> > ...
> > Now, obviously these types of conflicts are easy enough to resolve, but
> > it's still annoying.  It seems that this would be substantially less
> > often a problem if we just split such lines to one file per
> > line.
>
> We did something similar not too long ago in configure.in (bfa6c5a0c),
> and it seems to have helped.  +1

Cool. Any opinion on whether to got for

OBJS = \
        dest.o \
        fastpath.o \
...

or

OBJS = dest.o \
        fastpath.o \
...

I'm mildly inclined to go for the former.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2019-10-29 16:31:11 -0400, Tom Lane wrote:
>> We did something similar not too long ago in configure.in (bfa6c5a0c),
>> and it seems to have helped.  +1

> Cool. Any opinion on whether to got for ...

Not here.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Michael Paquier-2
In reply to this post by Andres Freund
On Tue, Oct 29, 2019 at 11:32:09PM -0700, Andres Freund wrote:

> Cool. Any opinion on whether to got for
>
> OBJS = \
> dest.o \
> fastpath.o \
> ...
>
> or
>
> OBJS = dest.o \
> fastpath.o \
> ...
>
> I'm mildly inclined to go for the former.
FWIW, I am more used to the latter, but the former is also fine by
me.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Mark Dilger-4
In reply to this post by Andres Freund


On 10/29/19 11:32 PM, Andres Freund wrote:

> Hi,
>
> On 2019-10-29 16:31:11 -0400, Tom Lane wrote:
>> Andres Freund <[hidden email]> writes:
>>> one of the most frequent conflicts I see is that two patches add files
>>> to OBJS (or one of its other spellings), and there are conflicts because
>>> another file has been added.
>>> ...
>>> Now, obviously these types of conflicts are easy enough to resolve, but
>>> it's still annoying.  It seems that this would be substantially less
>>> often a problem if we just split such lines to one file per
>>> line.
>>
>> We did something similar not too long ago in configure.in (bfa6c5a0c),
>> and it seems to have helped.  +1
>
> Cool. Any opinion on whether to got for
>
> OBJS = \
> dest.o \
> fastpath.o \
> ...
>
> or
>
> OBJS = dest.o \
> fastpath.o \
> ...
>
> I'm mildly inclined to go for the former.

+1 for the former.

>
> Greetings,
>
> Andres Freund
>
>


Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Andres Freund
In reply to this post by Andres Freund
Hi,

On 2019-10-29 23:32:09 -0700, Andres Freund wrote:

> On 2019-10-29 16:31:11 -0400, Tom Lane wrote:
> > Andres Freund <[hidden email]> writes:
> > > one of the most frequent conflicts I see is that two patches add files
> > > to OBJS (or one of its other spellings), and there are conflicts because
> > > another file has been added.
> > > ...
> > > Now, obviously these types of conflicts are easy enough to resolve, but
> > > it's still annoying.  It seems that this would be substantially less
> > > often a problem if we just split such lines to one file per
> > > line.
> >
> > We did something similar not too long ago in configure.in (bfa6c5a0c),
> > and it seems to have helped.  +1
>
> Cool. Any opinion on whether to got for
>
> OBJS = \
> dest.o \
> fastpath.o \
> ...
>
> or
>
> OBJS = dest.o \
> fastpath.o \
> ...
>
> I'm mildly inclined to go for the former.

Pushed a patch going with the former. Let's see what the buildfarm
says...

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Michael Paquier-2
On Tue, Nov 05, 2019 at 02:47:55PM -0800, Andres Freund wrote:
> Pushed a patch going with the former. Let's see what the buildfarm
> says...

Thanks Andres.  On a rather related note, would it make sense to do
the same for regression and isolation tests in our in-core modules?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> On Tue, Nov 05, 2019 at 02:47:55PM -0800, Andres Freund wrote:
>> Pushed a patch going with the former. Let's see what the buildfarm
>> says...

> Thanks Andres.  On a rather related note, would it make sense to do
> the same for regression and isolation tests in our in-core modules?

I don't think it'd be a great idea to change parallel_schedule like
that.  Independently adding test scripts to the same parallel batch
probably won't end well: you might end up over the concurrency limit,
or the scripts might conflict through sharing table names or the like.
So I'd rather see that there's a conflict to worry about.

Anyway, merge conflicts there aren't so common IME.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Andres Freund
In reply to this post by Michael Paquier-2
Hi,

On 2019-11-07 11:24:37 +0900, Michael Paquier wrote:
> On Tue, Nov 05, 2019 at 02:47:55PM -0800, Andres Freund wrote:
> > Pushed a patch going with the former. Let's see what the buildfarm
> > says...
>
> Thanks Andres.  On a rather related note, would it make sense to do
> the same for regression and isolation tests in our in-core modules?

I don't see them as being frequent sources of conflicts (partially
because we don't change linebreaks due to line length limits, I think),
so I don't think it's really worthwhile.

One I could see some benefit in, would be the SUBDIRS makefile lines.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: RFC: split OBJS lines to one object per line

Michael Paquier-2
In reply to this post by Tom Lane-2
On Thu, Nov 07, 2019 at 12:02:04PM -0500, Tom Lane wrote:
> I don't think it'd be a great idea to change parallel_schedule like
> that.  Independently adding test scripts to the same parallel batch
> probably won't end well: you might end up over the concurrency limit,
> or the scripts might conflict through sharing table names or the like.
> So I'd rather see that there's a conflict to worry about.
>
> Anyway, merge conflicts there aren't so common IME.

FWIW, I was not referring to the schedule files here, just to REGRESS
and ISOLATION in the modules' Makefiles.  If you think that's not
worth doing it, let's drop my suggestion then.
--
Michael

signature.asc (849 bytes) Download Attachment