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
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. +1Cool. 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.
Show quoted text
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. +1Cool. 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
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
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
Attachments:
Fixed_backend_folder_makefiles_v1.patchapplication/octet-stream; name=Fixed_backend_folder_makefiles_v1.patchDownload
commit 24917b50a56617928b6e6c3352cf44e4e599524f
Author: Mahendra Singh Thalor <mahi6run@gmail.com>
Date: Tue Dec 17 23:20:34 2019 +0530
Fixed some inconsistency into makefiles for one-line-per-entry style
diff --git a/src/backend/tsearch/Makefile b/src/backend/tsearch/Makefile
index 24fbcf7..b5bf854 100644
--- a/src/backend/tsearch/Makefile
+++ b/src/backend/tsearch/Makefile
@@ -24,21 +24,21 @@ DICTFILES=synonym_sample.syn thesaurus_sample.ths \
DICTFILES_PATH=$(addprefix dicts/,$(DICTFILES))
OBJS = \
- ts_locale.o \
- ts_parse.o \
- wparser.o \
- wparser_def.o \
dict.o \
+ dict_ispell.o \
dict_simple.o \
dict_synonym.o \
dict_thesaurus.o \
- dict_ispell.o \
regis.o \
spell.o \
to_tsany.o \
+ ts_locale.o \
+ ts_parse.o \
ts_selfuncs.o \
ts_typanalyze.o \
- ts_utils.o
+ ts_utils.o \
+ wparser.o \
+ wparser_def.o \
include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/utils/Makefile b/src/backend/utils/Makefile
index c2618e5..970b8ce 100644
--- a/src/backend/utils/Makefile
+++ b/src/backend/utils/Makefile
@@ -13,7 +13,8 @@ subdir = src/backend/utils
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
-OBJS = fmgrtab.o
+OBJS = \
+ fmgrtab.o
SUBDIRS = adt cache error fmgr hash init mb misc mmgr resowner sort time
# location of Catalog.pm
diff --git a/src/pl/plpython/Makefile b/src/pl/plpython/Makefile
index 667a744..0aebfd7 100644
--- a/src/pl/plpython/Makefile
+++ b/src/pl/plpython/Makefile
@@ -20,6 +20,7 @@ PGFILEDESC = "PL/Python - procedural language"
NAME = plpython$(python_majorversion)
OBJS = \
+ $(WIN32RES)
plpy_cursorobject.o \
plpy_elog.o \
plpy_exec.o \
@@ -32,7 +33,6 @@ OBJS = \
plpy_subxactobject.o \
plpy_typeio.o \
plpy_util.o \
- $(WIN32RES)
DATA = $(NAME)u.control $(NAME)u--1.0.sql $(NAME)u--unpackaged--1.0.sql
ifeq ($(python_majorversion),2)
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
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