Thread: RFC: split OBJS lines to one object per line
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
On Tue, Oct 29, 2019 at 1:09 PM Andres Freund <andres@anarazel.de> wrote: > Comments? I think that this is a good idea. I see no downside. -- Peter Geoghegan
Andres Freund <andres@anarazel.de> 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
Hi,
On 2019-10-29 16:31:11 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> 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
Andres Freund <andres@anarazel.de> 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
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
Attachment
On 10/29/19 11:32 PM, Andres Freund wrote: > Hi, > > On 2019-10-29 16:31:11 -0400, Tom Lane wrote: >> Andres Freund <andres@anarazel.de> 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 > >
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 <andres@anarazel.de> 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
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
Attachment
Michael Paquier <michael@paquier.xyz> 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
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
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
Attachment
On Fri, 8 Nov 2019 at 14:38, Michael Paquier <michael@paquier.xyz> wrote: > > 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. > -- I found some inconsistency in alphabetical order in src/backend/tsearch/Makefile, src/backend/utils/Makefile and src/pl/plpython/Makefile files. Attached patch is fixing those order related inconsistency. Thanks and Regards Mahendra Thalor EnterpriseDB: http://www.enterprisedb.com
Attachment
On Tue, Dec 17, 2019 at 11:40:17PM +0530, Mahendra Singh wrote: > I found some inconsistency in alphabetical order in > src/backend/tsearch/Makefile, src/backend/utils/Makefile and > src/pl/plpython/Makefile files. Attached patch is fixing those order > related inconsistency. Thanks, committed. The one-liner style is also used in ifaddrs, but fmgrtab.c is generated so I have left that out. Now, have you tried to compile plpython before sending this patch? Because as you forgot to add one backslash after WIN32RES, compilation was failing there. And you also forgot to remove two backslashes at the end of the other two lists modified :) -- Michael
Attachment
On Wed, 18 Dec 2019 at 07:23, Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Dec 17, 2019 at 11:40:17PM +0530, Mahendra Singh wrote: > > I found some inconsistency in alphabetical order in > > src/backend/tsearch/Makefile, src/backend/utils/Makefile and > > src/pl/plpython/Makefile files. Attached patch is fixing those order > > related inconsistency. > > Thanks, committed. The one-liner style is also used in ifaddrs, but Thanks Michael for quick response. > fmgrtab.c is generated so I have left that out. Now, have you tried > to compile plpython before sending this patch? Because as you forgot > to add one backslash after WIN32RES, compilation was failing there. > And you also forgot to remove two backslashes at the end of the other > two lists modified :) Sorry, I forgot to add backslashes. I will take care from next time. Thanks and Regards Mahendra Thalor EnterpriseDB: http://www.enterprisedb.com