Обсуждение: Re: [COMMITTERS] pgsql: pgindent run for 9.4
Bruce Momjian <bruce@momjian.us> wrote: > pgindent run for 9.4 > > This includes removing tabs after periods in C comments, which was > applied to back branches, so this change should not effect backpatching. > http://git.postgresql.org/pg/commitdiff/0a7832005792fa6dad171f9cadb8d587fe0dd800 > .../test/expected/compat_informix-test_informix2.c | 2 +- > src/interfaces/ecpg/test/expected/preproc-init.c | 2 +- > src/interfaces/ecpg/test/expected/sql-array.c | 2 +- > src/interfaces/ecpg/test/expected/sql-code100.c | 2 +- > src/interfaces/ecpg/test/expected/sql-copystdout.c | 2 +- > src/interfaces/ecpg/test/expected/sql-define.c | 2 +- > src/interfaces/ecpg/test/expected/sql-dynalloc.c | 2 +- > src/interfaces/ecpg/test/expected/sql-dynalloc2.c | 2 +- > src/interfaces/ecpg/test/expected/sql-dyntest.c | 2 +- > src/interfaces/ecpg/test/expected/sql-indicators.c | 2 +- > src/interfaces/ecpg/test/expected/thread-alloc.c | 2 +- > .../ecpg/test/expected/thread-descriptor.c | 2 +- > src/interfaces/ecpg/test/expected/thread-prep.c | 2 +- The 13 tests above are broken by this commit. Probably the directory should be excluded from pgindent processing. While I haven't checked, I assume all other branches are affected. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Kevin Grittner <kgrittn@ymail.com> writes:
> Bruce Momjian <bruce@momjian.us> wrote:
>> pgindent run for 9.4
> The 13 tests above are broken by this commit.� Probably the
> directory should be excluded from pgindent processing.
What's broken? The buildfarm isn't complaining, and "make installcheck"
in src/interfaces/ecpg/test passes for me.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > Kevin Grittner <kgrittn@ymail.com> writes: > >> Bruce Momjian <bruce@momjian.us> wrote: >>> pgindent run for 9.4 > >> The 13 tests above are broken by this commit. Probably the >> directory should be excluded from pgindent processing. > > What's broken? The buildfarm isn't complaining, and "make installcheck" > in src/interfaces/ecpg/test passes for me. On "make check-world" I get the attached. After the period, the trailing tabs in the comment have been changed to spaces in "expected", but are still tabs in "results". -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
On Tue, May 6, 2014 at 12:38:41PM -0700, Kevin Grittner wrote: > Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Kevin Grittner <kgrittn@ymail.com> writes: > > > >> Bruce Momjian <bruce@momjian.us> wrote: > >>> pgindent run for 9.4 > > > >> The 13 tests above are broken by this commit. Probably the > >> directory should be excluded from pgindent processing. > > > > What's broken? The buildfarm isn't complaining, and "make installcheck" > > in src/interfaces/ecpg/test passes for me. > > On "make check-world" I get the attached. After the period, the > trailing tabs in the comment have been changed to spaces in > "expected", but are still tabs in "results". Yes, I had to modify those lines before I pushed the pgindent changes so 'make installcheck-world' would pass. It passes here for me now. Did you do 'make maintainer-clean' before running the tests? That might help. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> wrote: > On Tue, May 6, 2014 at 12:38:41PM -0700, Kevin Grittner wrote: >> Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Kevin Grittner <kgrittn@ymail.com> writes: >>> >>>> Bruce Momjian <bruce@momjian.us> wrote: >>>>> pgindent run for 9.4 >>> >>>> The 13 tests above are broken by this commit. Probably the >>>> directory should be excluded from pgindent processing. >>> >>> What's broken? The buildfarm isn't complaining, and "make installcheck" >>> in src/interfaces/ecpg/test passes for me. >> >> On "make check-world" I get the attached. After the period, the >> trailing tabs in the comment have been changed to spaces in >> "expected", but are still tabs in "results". > > Yes, I had to modify those lines before I pushed the pgindent changes so > 'make installcheck-world' would pass. It passes here for me now. Did > you do 'make maintainer-clean' before running the tests? That might > help. It occurred to me after my last post that I had just done a "make world" without any cleanup when I pulled that, and had started a full build from "make maintainer-clean" when you sent that. :-) I'll let you know either way when I get results from that. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Kevin Grittner <kgrittn@ymail.com> wrote: > It occurred to me after my last post that I had just done a "make > world" without any cleanup when I pulled that, and had started a > full build from "make maintainer-clean" when you sent that. :-) > > I'll let you know either way when I get results from that. Yeah, after "make maintainer-clean" it's fine. Another case where --enable-depend doesn't handle things. I don't know whether we can or should try to fix that. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Kevin Grittner <kgrittn@ymail.com> writes:
> Bruce Momjian <bruce@momjian.us> wrote:
>> Yes, I had to modify those lines before I pushed the pgindent changes so
>> 'make installcheck-world' would pass.� It passes here for me now.� Did
>> you do 'make maintainer-clean' before running the tests?� That might
>> help.
> It occurred to me after my last post that I had just done a "make
> world" without any cleanup when I pulled that, and had started a
> full build from "make maintainer-clean" when you sent that.� :-)
FWIW, I did a "make distclean" before pulling the update, which is
my usual habit, and it worked fine. A look at the make rules for
ecpg suggests that "make clean" is enough to get rid of all the
derived files for the tests.
But having said that, if this didn't work then there's something broken
about the make rules for the ecpg tests. I'm a bit suspicious of commit
69e9768e7b183d4b276d0e067a5a0000689580eb.
regards, tom lane
On Tue, May 6, 2014 at 03:54:24PM -0400, Tom Lane wrote: > Kevin Grittner <kgrittn@ymail.com> writes: > > Bruce Momjian <bruce@momjian.us> wrote: > >> Yes, I had to modify those lines before I pushed the pgindent changes so > >> 'make installcheck-world' would pass.� It passes here for me now.� Did > >> you do 'make maintainer-clean' before running the tests?� That might > >> help. > > > It occurred to me after my last post that I had just done a "make > > world" without any cleanup when I pulled that, and had started a > > full build from "make maintainer-clean" when you sent that.� :-) > > FWIW, I did a "make distclean" before pulling the update, which is > my usual habit, and it worked fine. A look at the make rules for > ecpg suggests that "make clean" is enough to get rid of all the > derived files for the tests. > > But having said that, if this didn't work then there's something broken > about the make rules for the ecpg tests. I'm a bit suspicious of commit > 69e9768e7b183d4b276d0e067a5a0000689580eb. What _is_ odd is that I had to change these files after the pgindent run in head, but _not_ in the back branches when I removed the tabs from comments. I assume there is something new in 9.4 about they way they are built. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Tue, May 6, 2014 at 03:54:24PM -0400, Tom Lane wrote:
>> But having said that, if this didn't work then there's something broken
>> about the make rules for the ecpg tests. I'm a bit suspicious of commit
>> 69e9768e7b183d4b276d0e067a5a0000689580eb.
I looked into this, and find that the cause of the problem is that
pgindent touched src/interfaces/ecpg/include/sqlca.h, which is copied
verbatim into preprocessed files by the ecpg preprocessor, so the expected
files had to change in tandem. This amounts to a dependency, but the make
rules don't know about it. Should they? That particular file changes so
seldom that it'd hardly be worth worrying about, but I'm not sure which
other files can get copied similarly.
> What _is_ odd is that I had to change these files after the pgindent run
> in head, but _not_ in the back branches when I removed the tabs from
> comments. I assume there is something new in 9.4 about they way they
> are built.
I'm confused by this statement. Your tab-adjustment commits in the back
branches also touched both sqlca.h and the ecpg expected files.
regards, tom lane
On Tue, May 6, 2014 at 05:05:00PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Tue, May 6, 2014 at 03:54:24PM -0400, Tom Lane wrote: > >> But having said that, if this didn't work then there's something broken > >> about the make rules for the ecpg tests. I'm a bit suspicious of commit > >> 69e9768e7b183d4b276d0e067a5a0000689580eb. > > I looked into this, and find that the cause of the problem is that > pgindent touched src/interfaces/ecpg/include/sqlca.h, which is copied > verbatim into preprocessed files by the ecpg preprocessor, so the expected > files had to change in tandem. This amounts to a dependency, but the make > rules don't know about it. Should they? That particular file changes so > seldom that it'd hardly be worth worrying about, but I'm not sure which > other files can get copied similarly. > > > What _is_ odd is that I had to change these files after the pgindent run > > in head, but _not_ in the back branches when I removed the tabs from > > comments. I assume there is something new in 9.4 about they way they > > are built. > > I'm confused by this statement. Your tab-adjustment commits in the back > branches also touched both sqlca.h and the ecpg expected files. They probably did in the back branches as I hit _all_ C files. I wonder if pgindent somehow skipped some of them. Ah, found it. There is an excludes pattern file list I had forgotten about; it has: /s_lock\.h$/ecpg/test/expected//snowball/libstemmer//ecpg/include/(sqlda|sqltypes)\.h$/ecpg/include/preproc/struct\.h$/pl/plperl/ppport\.h$ I am thinking I should back out the tab/comment changes in those files in the back branches, though I would then need to adjust the ecpg regression tests. In practice, these files are rarely patched, so it might be fine to just leave them alone. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
I wrote:
> I looked into this, and find that the cause of the problem is that
> pgindent touched src/interfaces/ecpg/include/sqlca.h, which is copied
> verbatim into preprocessed files by the ecpg preprocessor, so the expected
> files had to change in tandem. This amounts to a dependency, but the make
> rules don't know about it. Should they? That particular file changes so
> seldom that it'd hardly be worth worrying about, but I'm not sure which
> other files can get copied similarly.
While I'm looking at it: there's no dependency forcing the test .c files
to get rebuilt after the ecpg preprocessor changes, either, and that
seems much more likely to be a routine problem.
Arguably, we need some more dependencies in this rule in
ecpg/test/Makefile.regress:
%.c: %.pgc ../regression.h$(ECPG) -o $@ -I$(srcdir) $<
I also notice that some of the subdirectory makefiles that include
Makefile.regress have custom build rules that seem mostly duplicative
of this one, except for passing different switches to ecpg. Those
would likewise need additions to their dependency lists, which suggests
that the "../regression.h" part ought to be wrapped up in some macro.
regards, tom lane
Bruce Momjian <bruce@momjian.us> writes:
> Ah, found it. There is an excludes pattern file list I had forgotten
> about; it has:
> /s_lock\.h$
> /ecpg/test/expected/
> /snowball/libstemmer/
> /ecpg/include/(sqlda|sqltypes)\.h$
> /ecpg/include/preproc/struct\.h$
> /pl/plperl/ppport\.h$
Ah, so you've been excluding some of the ecpg/include/ header files but
not sqlca.h.
> I am thinking I should back out the tab/comment changes in those files
> in the back branches, though I would then need to adjust the ecpg
> regression tests. In practice, these files are rarely patched, so it
> might be fine to just leave them alone.
No, let's not back them out. The real question here is why sqlca.h is
treated differently from those other three. At least in HEAD, I'd be
inclined to pgindent all of ecpg/include/ and just deal with any ensuing
test fallout. As long as updating the expected files is part of your
pgindent procedure, why not?
IOW, I get the reasons for those other exclusions:
s_lock.h: lots of inline ASM which pgindent doesn't deal well with
/snowball/libstemmer/: upstream code not maintained by us
ppport.h: ditto
But I don't see the reason why we shouldn't expect ecpg's headers to
conform to our layout rules.
regards, tom lane
On Tue, May 6, 2014 at 05:35:15PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Ah, found it. There is an excludes pattern file list I had forgotten > > about; it has: > > > /s_lock\.h$ > > /ecpg/test/expected/ > > /snowball/libstemmer/ > > /ecpg/include/(sqlda|sqltypes)\.h$ > > /ecpg/include/preproc/struct\.h$ > > /pl/plperl/ppport\.h$ > > Ah, so you've been excluding some of the ecpg/include/ header files but > not sqlca.h. > > > I am thinking I should back out the tab/comment changes in those files > > in the back branches, though I would then need to adjust the ecpg > > regression tests. In practice, these files are rarely patched, so it > > might be fine to just leave them alone. > > No, let's not back them out. The real question here is why sqlca.h is > treated differently from those other three. At least in HEAD, I'd be > inclined to pgindent all of ecpg/include/ and just deal with any ensuing > test fallout. As long as updating the expected files is part of your > pgindent procedure, why not? > > IOW, I get the reasons for those other exclusions: > > s_lock.h: lots of inline ASM which pgindent doesn't deal well with > /snowball/libstemmer/: upstream code not maintained by us > ppport.h: ditto > > But I don't see the reason why we shouldn't expect ecpg's headers to > conform to our layout rules. I don't know who ecpg got in there. Let me know what you would like done. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Tue, May 6, 2014 at 05:35:15PM -0400, Tom Lane wrote:
>> But I don't see the reason why we shouldn't expect ecpg's headers to
>> conform to our layout rules.
> I don't know who ecpg got in there. Let me know what you would like
> done.
What I'm suggesting is that, in HEAD only, you remove these exclusion
entries:
> /ecpg/include/(sqlda|sqltypes)\.h$
> /ecpg/include/preproc/struct\.h$
then redo the pgindent run (presumably only those three files will change)
and make any necessary updates in the ecpg expected files.
Note that it's just chance that the back branch updates didn't hit those
three files already, since you said you weren't using the filter on them.
regards, tom lane
On Tue, May 6, 2014 at 06:24:47PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Tue, May 6, 2014 at 05:35:15PM -0400, Tom Lane wrote: > >> But I don't see the reason why we shouldn't expect ecpg's headers to > >> conform to our layout rules. > > > I don't know who ecpg got in there. Let me know what you would like > > done. > > What I'm suggesting is that, in HEAD only, you remove these exclusion > entries: > > > /ecpg/include/(sqlda|sqltypes)\.h$ > > /ecpg/include/preproc/struct\.h$ > > then redo the pgindent run (presumably only those three files will change) > and make any necessary updates in the ecpg expected files. OK, done. ecpg exclusion removed, pgindent rerun (it only changed sqlda.h), and ecpg regression tests updated, all only in head. > Note that it's just chance that the back branch updates didn't hit those > three files already, since you said you weren't using the filter on them. Right. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +