Patchwork Ping^2: RFA (build): Avoid circular dependency

login
register
mail settings
Submitter Paolo Bonzini
Date Nov. 12, 2010, 2:33 p.m.
Message ID <4CDD502D.9060405@gnu.org>
Download mbox | patch
Permalink /patch/70972/
State New
Headers show

Comments

Paolo Bonzini - Nov. 12, 2010, 2:33 p.m.
On 11/12/2010 03:13 PM, Joern Rennecke wrote:
>> I don't have the sources at hand, so I cannot see right now what is
>> the full chain of dependencies.
>>
>> I'll review the patch early next week.
> 
> Are you still planning to do the review in the near future?

Yes.  Your patch has the disadvantage that a normal "make" will not
do the check anymore, because you removed the dependency from all to
s-tm-texi (which previously went all -> info -> gccint.info ->
tm.texi -> s-tm-texi).

What about this instead, which breaks the circular dependency by
making tm.texi phony.  This way "$(MAKE) tm.texi" is effectively
a synonym of "$(MAKE s-tm-texi)".


Does this work for you?

Paolo
Joern Rennecke - Nov. 12, 2010, 3:02 p.m.
Quoting Paolo Bonzini <bonzini@gnu.org>:

> On 11/12/2010 03:13 PM, Joern Rennecke wrote:
...
>> Are you still planning to do the review in the near future?
>
> Yes.  Your patch has the disadvantage that a normal "make" will not
> do the check anymore, because you removed the dependency from all to
> s-tm-texi (which previously went all -> info -> gccint.info ->
> tm.texi -> s-tm-texi).

We still have a gccint.info -> new-tm.texi -> s-tm-texi link.

> What about this instead, which breaks the circular dependency by
> making tm.texi phony.  This way "$(MAKE) tm.texi" is effectively
> a synonym of "$(MAKE s-tm-texi)".
>
> --- Makefile.in	(revision 166517)
> +++ Makefile.in	(working copy)
> @@ -3688,7 +3688,10 @@ s-constrs-h: $(MD_DEPS) build/genpreds$(
>  	$(STAMP) s-constrs-h
>
>  target-hooks-def.h: s-target-hooks-def-h; @true
> -tm.texi: s-tm-texi; @true
> +
> +all: tm.texi
> +tm.texi: force
> +	$(MAKE) s-tm-texi

This looks unsafe in a parallel make context.
make could decide to build tm.texi and s-tm-texi simultaneously.
Then in order to build tm.texi, it will invoke another make that
builds s-tm-texi, too.
Paolo Bonzini - Nov. 12, 2010, 3:06 p.m.
On 11/12/2010 04:02 PM, Joern Rennecke wrote:
> Quoting Paolo Bonzini <bonzini@gnu.org>:
>
>> On 11/12/2010 03:13 PM, Joern Rennecke wrote:
> ...
>>> Are you still planning to do the review in the near future?
>>
>> Yes. Your patch has the disadvantage that a normal "make" will not
>> do the check anymore, because you removed the dependency from all to
>> s-tm-texi (which previously went all -> info -> gccint.info ->
>> tm.texi -> s-tm-texi).
>
> We still have a gccint.info -> new-tm.texi -> s-tm-texi link.

Ok, I missed the last hunk.  That is unfortunately wrong because you 
would force people who compile a release tarball to run makeinfo 
(because new-tm.texi) is not shipped.

I think your patch can be okay if you leave TEXI_GCCINT_FILES aside, and 
put an additional dependency

@MAINT@	all: new-tm.texi

Paolo
Joern Rennecke - Nov. 12, 2010, 3:51 p.m.
Quoting Paolo Bonzini <bonzini@gnu.org>:

> I think your patch can be okay if you leave TEXI_GCCINT_FILES aside,
> and put an additional dependency
>
> @MAINT@	all: new-tm.texi

That has some other drawbacks - you could end up using inconsistent / out-of
date documentation if you haven't explicitly specified maintainer mode.

I think the solution is to have the .info files depend in the stamp file,
not on tm.texi or new-tm.texi.
In fact, I think I already did this before, in version that reduced
unnecessary rebuilds by paying more attention to stamp file usage -
I just thought it'll be easier to review these Makefile changes in
smaller increments.
But if a partial fix involves another regression, it's probably better to
fix the entire thing.  I'll go digging for what I had there.
Joern Rennecke - Nov. 12, 2010, 4 p.m.
P.S.: Of course it's the other way round:
doc/tm.texi depends on the stampfile, the info files depend on doc/tm.texi .
If the documentation is up-to-date, the stampfile gets made without
complaint, and doc/tm.texi won't get touched, so if your info files are newer
than that, they won't need top be rebuilt.
Paolo Bonzini - Nov. 12, 2010, 4:01 p.m.
On 11/12/2010 04:51 PM, Joern Rennecke wrote:
> But if a partial fix involves another regression, it's probably better to
> fix the entire thing.  I'll go digging for what I had there.

Sure.

Paolo
Joern Rennecke - Nov. 14, 2010, 2:46 p.m.
Quoting Joern Rennecke <amylaar@spamcop.net>:

> P.S.: Of course it's the other way round:
> doc/tm.texi depends on the stampfile,
Oops, there is the circular dependency again.

What we actually need is a timestamp that says that the consistency has been
checked, and which depends on doc/tm.texi - that's our current s-tm-texi -
plus a generated file that depends on s-tm-texi, but bears the file system
time stamp of doc/tm.texi .
I.e. the s-tm-texi rule should make this file.
Can we use cp -p for this portably?
In principle we could also use some combination of time and ls and shell
substitutions, that would use less disk space, but I fear the portability
issues there could be worse.

Or should I better try to use double-colon rules?

I.e. TEXI_GCCINT_FILES would include [doc/]tm.texi, but all the files that
depend on TEXI_GCCINT_FILES would do so with a double-colon rule, and in
an earlier double-colon rule, they'd all depend on s-tm-texi, with empty
commands, to make sure doc/tm.texi is up-to-date.
Using double-colon rules would avoid portability concerns how to copy a
timestamp, but would force as to write all the commands to generate the
files that depend on tm.texi in the rule that state the dependency.
Ralf Wildenhues - Nov. 14, 2010, 3:27 p.m.
* Joern Rennecke wrote on Sun, Nov 14, 2010 at 03:46:29PM CET:
> What we actually need is a timestamp that says that the consistency has been
> checked, and which depends on doc/tm.texi - that's our current s-tm-texi -
> plus a generated file that depends on s-tm-texi, but bears the file system
> time stamp of doc/tm.texi .
> I.e. the s-tm-texi rule should make this file.
> Can we use cp -p for this portably?

Not likely; see the discussion in 'info Autoconf --index cp'.

> In principle we could also use some combination of time and ls and shell
> substitutions, that would use less disk space, but I fear the portability
> issues there could be worse.

Yes, that sounds a lot worse.

> Or should I better try to use double-colon rules?

If that helps, I suppose yes; we assume GNU make in this makefile
anyway.

Hope that helps.

Cheers,
Ralf
Joern Rennecke - Nov. 14, 2010, 3:35 p.m.
Quoting Ralf Wildenhues <Ralf.Wildenhues@gmx.de>:

> * Joern Rennecke wrote on Sun, Nov 14, 2010 at 03:46:29PM CET:
...
>> Or should I better try to use double-colon rules?
>
> If that helps, I suppose yes; we assume GNU make in this makefile
> anyway.

I just realized there is a third alternative: we could exploit the fact
that the same file is handled by make as different targets if named
with different pathnames, i.e.
[$(srcdir)/doc/]tm.texi and $(srcdir)/doc/../doc/tm.texi are different  
make targets.
I'm not sure if that would be exploiting a stable feature or a bug/limitation
that might disappear from GNU make before the FSF GPL/GFDL problems
are resolved.

If we could use a file as differetn make targets in this fashion, that would
be a lot less pain to implement than using double-colon rules.
Ralf Wildenhues - Nov. 14, 2010, 3:46 p.m.
* Joern Rennecke wrote on Sun, Nov 14, 2010 at 04:35:02PM CET:
> I just realized there is a third alternative: we could exploit the fact
> that the same file is handled by make as different targets if named
> with different pathnames, i.e.
> [$(srcdir)/doc/]tm.texi and $(srcdir)/doc/../doc/tm.texi are
> different make targets.
> I'm not sure if that would be exploiting a stable feature or a bug/limitation
> that might disappear from GNU make before the FSF GPL/GFDL problems
> are resolved.

It's not likely that this will disappear, but code like that will jump
out at the casual reader months from now as likely-buggy.  So that would
definitely deserve a comment.  Maybe also a make-time (or
configure-time) test that this feature/bug has not changed?

Note also that in case you're playing a similar game with ./file vs.
$(srcdir)/file that will hurt in-tree builds of GCC (which, while
advised against, would still be nice to have working).

Cheers,
Ralf
Joern Rennecke - Nov. 14, 2010, 3:51 p.m.
Quoting Ralf Wildenhues <Ralf.Wildenhues@gmx.de>:

> * Joern Rennecke wrote on Sun, Nov 14, 2010 at 03:46:29PM CET:
>> What we actually need is a timestamp that says that the consistency has been
>> checked, and which depends on doc/tm.texi - that's our current s-tm-texi -
>> plus a generated file that depends on s-tm-texi, but bears the file system
>> time stamp of doc/tm.texi .
>> I.e. the s-tm-texi rule should make this file.
>> Can we use cp -p for this portably?
>
> Not likely; see the discussion in 'info Autoconf --index cp'.

This talks about the possibility that the timestamp might end up appearing
up to one second too old.  So in theory, you could end up with the info
files not being rebuilt even though the .texi file is newer.
In practice, the info files would need to be rebuilt if someone changes
one of the input files of $(builddir)/{new-,}tm.texi, rebuilds {new-,}tm.texi,
and copies it manually to $(srcdir)/doc/tm.texi.
A turn around time of less than a second for that process seems rather
unlikely, so if the only worry about cp -p is truncating the time to look
up to a second older, I think it is portable enough for this purpose.
Nathanael Nerode (GCC) - Nov. 15, 2010, 2:52 a.m.
On 11/14/2010 09:46 AM, Joern Rennecke wrote:
> Quoting Joern Rennecke <amylaar@spamcop.net>:
> 
>> P.S.: Of course it's the other way round:
>> doc/tm.texi depends on the stampfile,
> Oops, there is the circular dependency again.
> 
> What we actually need is a timestamp that says that the consistency has
> been
> checked, and which depends on doc/tm.texi - that's our current s-tm-texi -
> plus a generated file that depends on s-tm-texi, but bears the file system
> time stamp of doc/tm.texi .
> I.e. the s-tm-texi rule should make this file.
> Can we use cp -p for this portably?
> In principle we could also use some combination of time and ls and shell
> substitutions, that would use less disk space, but I fear the portability
> issues there could be worse.

Just in case anyone's looking for a good project, I figured out a VERY
long time ago that the true long-term solution to practically all
problems I've ever encountered in Makefile design, including this one,
lies in creating an extension to "make".  Unfortunately I've never
gotten around to digging into the internals of anyone's version of
'make' in order to figure out where to implement this, and the GNU make
people were completely uninterested, since they didn't get why it was a
good idea.

'make' badly needs a mechanism whereby the Makefile programmer can
specify a code fragment for a given target A which answers the question
'is A up to date?'.  The default implementation would be to check the
timestamp relative to the timestamps of the dependencies, but the
Makefile programmer should *be able to specify arbitrary rules*.  One
would want them to be fast, but that's the Makefile programmer's
problem.  :-)  They should be able to depend on the boolean answers to
the question of whether the prereqs are up to date.

It would be easier to do this in make than by the generation of many
artificial, breakage-prone timestamping and phony targets; it would also
render the majority of GNU make extensions irrelevant.

In the meantime, Joern's patch of 2010-11-14 looks good to me.
Paolo Bonzini - Nov. 15, 2010, 9:13 a.m.
On 11/15/2010 03:52 AM, Nathanael Nerode (GCC) wrote:
> In the meantime, Joern's patch of 2010-11-14 looks good to me.

Joern, take this as an approval.

Nathanael, welcome back!

Paolo
Ralf Wildenhues - Nov. 20, 2010, 9 p.m.
Hello Nathanael,

* Nathanael Nerode (GCC) wrote on Mon, Nov 15, 2010 at 03:52:44AM CET:
> Just in case anyone's looking for a good project, I figured out a VERY
> long time ago that the true long-term solution to practically all
> problems I've ever encountered in Makefile design, including this one,
> lies in creating an extension to "make".  Unfortunately I've never
> gotten around to digging into the internals of anyone's version of
> 'make' in order to figure out where to implement this, and the GNU make
> people were completely uninterested, since they didn't get why it was a
> good idea.

> 'make' badly needs a mechanism whereby the Makefile programmer can
> specify a code fragment for a given target A which answers the question
> 'is A up to date?'.

I assume you are hinting at
<http://thread.gmane.org/gmane.comp.gnu.make.bugs/1007>.

The reply from Paul is still mostly correct though: make walks the
complete dependency tree down and then back up.  When walking back up,
the mechanism you desire is easily implemented in a small shell command
(as shown in the example of Paul's reply).

When first walking down, you just need to ensure that the graph really
is a tree.  There are (at least) two choices you can make at that time,
and in GNU make you can implement them with arbitrary shell statements:

- should T be updated if P is newer?

    T : $(shell if $$condition; then echo P; fi)

- should T, if it is to be updated, be updated after P is updated
  (order-only dependency, since GNU make 3.80)?

    T : | $(shell if $$condition; then echo P; fi)

So, in summary, I think what you want is already possible, albeit maybe
not in a concise notation.

Cheers,
Ralf
Nathanael Nerode (GCC) - Nov. 21, 2010, 11:53 p.m.
On 11/20/2010 04:00 PM, Ralf Wildenhues wrote:
> The reply from Paul is still mostly correct though: make walks the
> complete dependency tree down and then back up.  When walking back up,
> the mechanism you desire is easily implemented in a small shell command
> (as shown in the example of Paul's reply).
It's dependent on timestamp files which can be poked, prodded, and
otherwise convinced to do the wrong thing by the user.  It's
fundamentally *not robust*, is the main issue.

> When first walking down, you just need to ensure that the graph really
> is a tree.  There are (at least) two choices you can make at that time,
> and in GNU make you can implement them with arbitrary shell statements:
>
> - should T be updated if P is newer?
>
>     T : $(shell if $$condition; then echo P; fi)

The problematic part is that you're scattering bogus timestamp files
(P!) all over the tree.  This makes it very brittle.  I suppose one
could construct a special subtree of timestamps which is blown away
fresh with each invocation.... that would make it *slightly more*
robust.  Not *much* more.

> - should T, if it is to be updated, be updated after P is updated
>   (order-only dependency, since GNU make 3.80)?
>
>     T : | $(shell if $$condition; then echo P; fi)
>
> So, in summary, I think what you want is already possible, albeit maybe
> not in a concise notation.

It's not so much the notation (although that is a problem) as the
scattering of bogus timestamp files.  It is true that by introducing an
auxilliary file to represent *each* logical state, and manipulating its
timestamp by hand to be "true" or "false", you can "solve" the problem,
but this is *not robust*, because the timestamp files are exposed to
general programmer manipulation.  A special timestamp subdirectory tree
would ameliorate that somewhat.

It is also, incidentally, slow, due to the heavy reliance on filesystem
access and modification, for things for which the filesystem is,
fundamentally, quite irrelevant.  I suppose it could be sped up with a
mounted ramdisk for the timestamp tree, but you begin to see how
ridiculous this set of kludges is.

Patch

--- Makefile.in	(revision 166517)
+++ Makefile.in	(working copy)
@@ -3688,7 +3688,10 @@  s-constrs-h: $(MD_DEPS) build/genpreds$(
 	$(STAMP) s-constrs-h
 
 target-hooks-def.h: s-target-hooks-def-h; @true
-tm.texi: s-tm-texi; @true
+
+all: tm.texi
+tm.texi: force
+	$(MAKE) s-tm-texi
 
 s-target-hooks-def-h: build/genhooks$(build_exeext)
 	$(RUN_GEN) build/genhooks$(build_exeext) > tmp-target-hooks-def.h