Re: dependencies for generated header files

Hi,

I think that our dependencies for generated header files (gram.h,
fmgroids.h, probes.h) are not as good as they could be. What we do
right now is make src/backend/Makefile rebuild these before recursing
through its subdirectories. This works OK for a top-level make, but
if you run make further down in the tree (like under
src/backend/commands) it won't necessarily rebuild everything that it
should.

The attached patch moves some of this logic from src/backend/Makefile
to src/Makefile.global.in. That way, if you --enable-depend and then
do something like "touch src/include/catalog/pg_proc.h" and then "cd
src/backend/commands; make vacuum.o", it rebuilds fmgroids.h and then
recompiles vacuum.c. Under HEAD, it just tells you that vacuum.o is
up to date.

I have tested this on vpath and non-vpath builds, with and without
--enable-depend.

...Robert

On Sun, Jun 28, 2009 at 2:21 PM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
> I think that our dependencies for generated header files (gram.h,
> fmgroids.h, probes.h) are not as good as they could be.  What we do
> right now is make src/backend/Makefile rebuild these before recursing
> through its subdirectories.  This works OK for a top-level make, but
> if you run make further down in the tree (like under
> src/backend/commands) it won't necessarily rebuild everything that it
> should.
>
> The attached patch moves some of this logic from src/backend/Makefile
> to src/Makefile.global.in.  That way, if you --enable-depend and then
> do something like "touch src/include/catalog/pg_proc.h" and then "cd
> src/backend/commands; make vacuum.o", it rebuilds fmgroids.h and then
> recompiles vacuum.c.  Under HEAD, it just tells you that vacuum.o is
> up to date.
>
> I have tested this on vpath and non-vpath builds, with and without
> --enable-depend.

Woops. It seems that patch generates some warnings on a vpath build
which I failed to notice. Corrected version that guards against same
is attached.

...Robert

Robert Haas escribió:

> Woops. It seems that patch generates some warnings on a vpath build
> which I failed to notice. Corrected version that guards against same
> is attached.

Interestingly, this patch causes diffstat (at least my version of it) to
attribute the line additions to src/backend/catalog/dependency.c instead
of the new file ...

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Sunday 28 June 2009 21:21:35 Robert Haas wrote:
> I think that our dependencies for generated header files (gram.h,
> fmgroids.h, probes.h) are not as good as they could be. What we do
> right now is make src/backend/Makefile rebuild these before recursing
> through its subdirectories. This works OK for a top-level make, but
> if you run make further down in the tree (like under
> src/backend/commands) it won't necessarily rebuild everything that it
> should.
>
> The attached patch moves some of this logic from src/backend/Makefile
> to src/Makefile.global.in.

Have you tried putting them in src/backend/common.mk? That might be a better
place.

Also, modulo that change, do you think it would be OK to apply this patch now
while the other patch about header generation is under discussion? Or would
that create a complete merge disaster if we ended up going with some variant
of the header generation patch?

On Wed, Jul 29, 2009 at 8:43 AM, Peter Eisentraut<peter_e(at)gmx(dot)net> wrote:
> On Sunday 28 June 2009 21:21:35 Robert Haas wrote:
>> I think that our dependencies for generated header files (gram.h,
>> fmgroids.h, probes.h) are not as good as they could be.  What we do
>> right now is make src/backend/Makefile rebuild these before recursing
>> through its subdirectories.  This works OK for a top-level make, but
>> if you run make further down in the tree (like under
>> src/backend/commands) it won't necessarily rebuild everything that it
>> should.
>>
>> The attached patch moves some of this logic from src/backend/Makefile
>> to src/Makefile.global.in.
>
> Have you tried putting them in src/backend/common.mk?  That might be a better
> place.

No, I haven't. I can look at that at some point, but I'm a bit backed
up right now.

> Also, modulo that change, do you think it would be OK to apply this patch now
> while the other patch about header generation is under discussion?  Or would
> that create a complete merge disaster if we ended up going with some variant
> of the header generation patch?

Well, since this one is only an 83-line patch, I don't think it will
be too bad. Of course if committing this gets you excited about
overhauling the Makefiles and you decide to change a bunch of other
things too, that might be more of an issue...

...Robert

On Wed, Jul 29, 2009 at 9:28 AM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jul 29, 2009 at 8:43 AM, Peter Eisentraut<peter_e(at)gmx(dot)net> wrote:
>> On Sunday 28 June 2009 21:21:35 Robert Haas wrote:
>>> I think that our dependencies for generated header files (gram.h,
>>> fmgroids.h, probes.h) are not as good as they could be.  What we do
>>> right now is make src/backend/Makefile rebuild these before recursing
>>> through its subdirectories.  This works OK for a top-level make, but
>>> if you run make further down in the tree (like under
>>> src/backend/commands) it won't necessarily rebuild everything that it
>>> should.
>>>
>>> The attached patch moves some of this logic from src/backend/Makefile
>>> to src/Makefile.global.in.
>>
>> Have you tried putting them in src/backend/common.mk?  That might be a better
>> place.
>
> No, I haven't.  I can look at that at some point, but I'm a bit backed
> up right now.

[ looks ]

Is it conceivable that there might be a dependency on one of the
generated header files from someplace in src other than src/backend?
If so, it seems like that might be an argument for leaving it as I had
it.

...Robert

Robert Haas escribió:

> Is it conceivable that there might be a dependency on one of the
> generated header files from someplace in src other than src/backend?
> If so, it seems like that might be an argument for leaving it as I had
> it.

Well, plperl.c includes fmgroids.h.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Sunday 28 June 2009 21:21:35 Robert Haas wrote:
> I think that our dependencies for generated header files (gram.h,
> fmgroids.h, probes.h) are not as good as they could be. What we do
> right now is make src/backend/Makefile rebuild these before recursing
> through its subdirectories. This works OK for a top-level make, but
> if you run make further down in the tree (like under
> src/backend/commands) it won't necessarily rebuild everything that it
> should.
>
> The attached patch moves some of this logic from src/backend/Makefile
> to src/Makefile.global.in. That way, if you --enable-depend and then
> do something like "touch src/include/catalog/pg_proc.h" and then "cd
> src/backend/commands; make vacuum.o", it rebuilds fmgroids.h and then
> recompiles vacuum.c. Under HEAD, it just tells you that vacuum.o is
> up to date.

I'm not convinced by this use case. Why do you want to rebuild vacuum.o
anyway? If you change something, then you probably want to rebuild postgres,
and that works fine without this change.

I'm concerned how this change moves random make rules into a global makefile.
This is exacerbated by your next patch, which among other things moves the
entire catalog list to build the various output files into Makefile.global,
far away from its home.

On Tue, Aug 11, 2009 at 4:38 PM, Peter Eisentraut<peter_e(at)gmx(dot)net> wrote:
> On Sunday 28 June 2009 21:21:35 Robert Haas wrote:
>> I think that our dependencies for generated header files (gram.h,
>> fmgroids.h, probes.h) are not as good as they could be.  What we do
>> right now is make src/backend/Makefile rebuild these before recursing
>> through its subdirectories.  This works OK for a top-level make, but
>> if you run make further down in the tree (like under
>> src/backend/commands) it won't necessarily rebuild everything that it
>> should.
>>
>> The attached patch moves some of this logic from src/backend/Makefile
>> to src/Makefile.global.in.  That way, if you --enable-depend and then
>> do something like "touch src/include/catalog/pg_proc.h" and then "cd
>> src/backend/commands; make vacuum.o", it rebuilds fmgroids.h and then
>> recompiles vacuum.c.  Under HEAD, it just tells you that vacuum.o is
>> up to date.
>
> I'm not convinced by this use case.  Why do you want to rebuild vacuum.o
> anyway?  If you change something, then you probably want to rebuild postgres,
> and that works fine without this change.

Well, my original motivation for developing this patch was that I
often go into a subdirectory and start hacking on a .c file and then
type make to rebuild just that subdirectory (so it's easier to see
warnings and errors). But sometimes I would edit a relevant header
file and then the rebuild wouldn't happen. That bugged me. Your
mileage may vary.

> I'm concerned how this change moves random make rules into a global makefile.
> This is exacerbated by your next patch, which among other things moves the
> entire catalog list to build the various output files into Makefile.global,
> far away from its home.

*shrug* You don't have to accept the patch, but I'm unclear as to how
make from a subdirectory will ever work reliably without something
like this. The problem occurs when .c files have dependencies on
files that are generated by a Makefile in some other subdirectory. So
it's not random stuff: it's the stuff where that particular thing
happens.

...Robert

Robert Haas escribió:

> *shrug* You don't have to accept the patch, but I'm unclear as to how
> make from a subdirectory will ever work reliably without something
> like this. The problem occurs when .c files have dependencies on
> files that are generated by a Makefile in some other subdirectory. So
> it's not random stuff: it's the stuff where that particular thing
> happens.

But what use would have to build that particular .o file, and not the
whole postgres binary?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Aug 11, 2009 at 4:38 PM, Peter Eisentraut<peter_e(at)gmx(dot)net> wrote:
>> I'm not convinced by this use case.

> Well, my original motivation for developing this patch was that I
> often go into a subdirectory and start hacking on a .c file and then
> type make to rebuild just that subdirectory (so it's easier to see
> warnings and errors). But sometimes I would edit a relevant header
> file and then the rebuild wouldn't happen. That bugged me. Your
> mileage may vary.

Surely the answer to that is "you should be configuring with --enable-depend".

regards, tom lane

On Tue, Aug 11, 2009 at 9:00 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Aug 11, 2009 at 4:38 PM, Peter Eisentraut<peter_e(at)gmx(dot)net> wrote:
>>> I'm not convinced by this use case.
>
>> Well, my original motivation for developing this patch was that I
>> often go into a subdirectory and start hacking on a .c file and then
>> type make to rebuild just that subdirectory (so it's easier to see
>> warnings and errors).  But sometimes I would edit a relevant header
>> file and then the rebuild wouldn't happen.  That bugged me.  Your
>> mileage may vary.
>
> Surely the answer to that is "you should be configuring with --enable-depend".

Uhm, the point is that this is broken even with ---enable-depend.
Please refer to the OP.

...Robert

On Tue, Aug 11, 2009 at 5:56 PM, Alvaro
Herrera<alvherre(at)commandprompt(dot)com> wrote:
> Robert Haas escribió:
>
>> *shrug*  You don't have to accept the patch, but I'm unclear as to how
>> make from a subdirectory will ever work reliably without something
>> like this.  The problem occurs when .c files have dependencies on
>> files that are generated by a Makefile in some other subdirectory.  So
>> it's not random stuff: it's the stuff where that particular thing
>> happens.
>
> But what use would have to build that particular .o file, and not the
> whole postgres binary?

I don't think that I can spell it out any more clearly than I did in
the paragraph immediately preceding the one you have quoted here.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Aug 11, 2009 at 9:00 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Surely the answer to that is "you should be configuring with --enable-depend".

> Uhm, the point is that this is broken even with ---enable-depend.

Oh, okay, but that's only for *generated* header files. I'm not excited
about that. We've pretty much managed to minimize compile-time
dependencies on generated files. Now of course your other patch wants
to vastly expand the use of generated files, and I can see that we might
have a problem of this sort if we accepted that patch --- but it seems
Peter's not excited about that one either.

regards, tom lane

On Tue, Aug 11, 2009 at 9:19 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Aug 11, 2009 at 9:00 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Surely the answer to that is "you should be configuring with --enable-depend".
>
>> Uhm, the point is that this is broken even with ---enable-depend.
>
> Oh, okay, but that's only for *generated* header files.  I'm not excited
> about that.  We've pretty much managed to minimize compile-time
> dependencies on generated files.  Now of course your other patch wants
> to vastly expand the use of generated files, and I can see that we might
> have a problem of this sort if we accepted that patch --- but it seems
> Peter's not excited about that one either.

Given that the anum.h stuff is gone, "vastly" might be an
overstatement. I'm pretty surprised to find out that people don't
like the idea of having dependencies be correct from anywhere in the
tree. Even if I'm the only developer who does partial builds, the
cost seems to me to be next to nil, so I'm not quite sure what anyone
gets out of rejecting this patch. That having been said, it's not
really worth it to me to spend a lot of time arguing about it. So
far, the only coherent argument why this is bad is that it moves some
logic into a shared Makefile rather than a directory-specific
Makefile, which might be confusing to someone trying to maintain the
Makefiles. I don't really buy that because they're already complex
enough that you have to read them all to understand what they are
doing, and nothing in this quite small patch is going to change that
picture very much, but I guess that's just me.

...Robert

Robert Haas escribió:

> Given that the anum.h stuff is gone, "vastly" might be an
> overstatement. I'm pretty surprised to find out that people don't
> like the idea of having dependencies be correct from anywhere in the
> tree. Even if I'm the only developer who does partial builds, the
> cost seems to me to be next to nil, so I'm not quite sure what anyone
> gets out of rejecting this patch.

I actually kinda like this patch. I tend to do partial builds
frequently.

> That having been said, it's not
> really worth it to me to spend a lot of time arguing about it. So
> far, the only coherent argument why this is bad is that it moves some
> logic into a shared Makefile rather than a directory-specific
> Makefile, which might be confusing to someone trying to maintain the
> Makefiles. I don't really buy that because they're already complex
> enough that you have to read them all to understand what they are
> doing, and nothing in this quite small patch is going to change that
> picture very much, but I guess that's just me.

The action-at-a-distance rules in the shared makefile is a pain, but I
think I'd live with it -- just make sure it is properly documented in
both places.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Given that the anum.h stuff is gone, "vastly" might be an
> overstatement. I'm pretty surprised to find out that people don't
> like the idea of having dependencies be correct from anywhere in the
> tree. Even if I'm the only developer who does partial builds, the
> cost seems to me to be next to nil, so I'm not quite sure what anyone
> gets out of rejecting this patch.

It's not that having the dependencies be 100% up to date wouldn't be
nice; it's that there's a limit to how much we're willing to uglify
the Makefiles to have that. The makefiles need maintenance too,
you know, and putting things far away from where they should be is
not any better in the makefiles than it is in C code.

As far as I can tell, if you've used --enable-depend then things will
get updated properly before you can ever attempt to run the code
(ie, install a rebuilt postmaster). The only situation where you'd
actually get an improvement from redoing the dependencies like this
is where lack of an update to a derived file results in a compiler
error/warning. But there aren't many such cases. The only one I can
even think of offhand is lack of an fmgroids.h symbol for a newly-added
function ... but we don't use F_XXX symbols enough to make that a
convincing example. We've intentionally arranged things so that
more-fragile cases like gram.h are not referenced outside their own
directories.

regards, tom lane

On Tue, Aug 11, 2009 at 9:56 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Given that the anum.h stuff is gone, "vastly" might be an
>> overstatement.  I'm pretty surprised to find out that people don't
>> like the idea of having dependencies be correct from anywhere in the
>> tree.  Even if I'm the only developer who does partial builds, the
>> cost seems to me to be next to nil, so I'm not quite sure what anyone
>> gets out of rejecting this patch.
>
> It's not that having the dependencies be 100% up to date wouldn't be
> nice; it's that there's a limit to how much we're willing to uglify
> the Makefiles to have that.  The makefiles need maintenance too,
> you know, and putting things far away from where they should be is
> not any better in the makefiles than it is in C code.

Well, I certainly agree that making a huge mess to address what is
admittedly a corner case is not a good idea. But I also don't think
this patch is all that messy. However, I guess we're getting to the
point where we need to make a decision one way or the other so that we
can close out this CommitFest.

> As far as I can tell, if you've used --enable-depend then things will
> get updated properly before you can ever attempt to run the code
> (ie, install a rebuilt postmaster).  The only situation where you'd
> actually get an improvement from redoing the dependencies like this
> is where lack of an update to a derived file results in a compiler
> error/warning.  But there aren't many such cases.  The only one I can
> even think of offhand is lack of an fmgroids.h symbol for a newly-added
> function ... but we don't use F_XXX symbols enough to make that a
> convincing example.  We've intentionally arranged things so that
> more-fragile cases like gram.h are not referenced outside their own
> directories.

Yes, that's definitely the best situation.

...Robert