Makefile target invalidated after config.status if runnning with make -C

RESOLVED FIXED in mozilla28

Status

defect
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: Ehsan, Assigned: gps)

Tracking

Trunk
mozilla28
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

So now when I touch content/media/webaudio/test/Makefile.in, and build in content/media, I get a "reticulating splines" message followed by the whole directory being rebuilt.  When I then build in dom/bindings, the whole directory get rebuilt again.  This is super annoying.  I _think_ it's a regression from the CPPSRCS move, or at least that's when I noticed it.
I think this is actually fallout from a change gps made, but I can't find the bug offhand.
Assignee

Comment 2

6 years ago
This is by design. The Makefile is listed as a global dependency for many targets in rules.mk, which means that touching Makefile.in or Makefile will invalidate previously-built targets. This is needed to ensure Makefile changes result in rebuilds.

The downside is that not all Makefile updates need to invalidate targets. We can be more intelligent about this. That will be done as part of other work and it isn't worth tracking in an explicit bug.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
(In reply to comment #2)
> This is by design. The Makefile is listed as a global dependency for many
> targets in rules.mk, which means that touching Makefile.in or Makefile will
> invalidate previously-built targets. This is needed to ensure Makefile changes
> result in rebuilds.

Is this going to go away in the moz.build world?

> The downside is that not all Makefile updates need to invalidate targets. We
> can be more intelligent about this. That will be done as part of other work and
> it isn't worth tracking in an explicit bug.

The problem here is that touching a Makefile.in in the "test" directory causes the contents of the parent to be rebuilt, which is a regression, and it's actually not needed for proper dependency tracking as far as I can tell.  Can you please tell me which other bug is tracking this fix?
Assignee

Comment 4

6 years ago
On deeper investigation, I can reproduce this. Likely regressed from bug 874543.
Status: RESOLVED → REOPENED
Depends on: 874543
Resolution: WONTFIX → ---
Assignee

Comment 5

6 years ago
STR:

$ ./config.status
$ make -s -C content/media
...
AudioAvailableEventManager.cpp
AudioChannelFormat.cpp
AudioNodeEngine.cpp
AudioNodeStream.cpp
AudioSegment.cpp
AudioStream.cpp
AudioStreamTrack.cpp
DOMMediaStream.cpp
DecoderTraits.cpp
FileBlockCache.cpp
MediaCache.cpp
MediaDecoder.cpp
MediaDecoderReader.cpp
MediaDecoderStateMachine.cpp
MediaResource.cpp
MediaStreamGraph.cpp
MediaStreamTrack.cpp
StreamBuffer.cpp
TextTrack.cpp
TextTrackCue.cpp
TextTrackCueList.cpp
TextTrackList.cpp
VideoFrameContainer.cpp
VideoSegment.cpp
VideoStreamTrack.cpp
VideoUtils.cpp
WebVTTLoadListener.cpp

Interestingly, if we run |make -C content| instead of |make -C content/media|, no rebuilding occurs! Huh?!
Assignee: nobody → gps
Status: REOPENED → ASSIGNED
Assignee

Comment 6

6 years ago
Woah:

$ ./config.status
$ make -s -C content
<no .cpp files are rebuilt>
$ make -s -C content/media
<the .cpp files from the previous comment are rebuilt>

?!
Assignee

Comment 7

6 years ago
So, Makefile is definitely causing invalidation here.

Prerequisite `Makefile' is newer than target `AudioAvailableEventManager.o'.

Now to figure out why.
Assignee

Comment 8

6 years ago
So, content/media/Makefile is being touched as part of |make -C content/media|. Digging into |make -d|:

  Prerequisite `../../backend.RecursiveMakeBackend.built' is newer than target `Makefile'.
 Must remake target `Makefile'.
make: Entering directory `/home/gps/src/firefox/obj-firefox/content/media'
Putting child 0x00c02320 (Makefile) PID 5188 on the chain.
Live child 0x00c02320 (Makefile) PID 5188 
Reaping winning child 0x00c02320 PID 5188 
Removing child 0x00c02320 PID 5188 from chain.
 Successfully remade target file `Makefile'.
make: Leaving directory `/home/gps/src/firefox/obj-firefox/content/media'
Re-executing[1]: make -d -C content/media

...

   No need to remake target `../../backend.RecursiveMakeBackend.built'.
  Finished prerequisites of target file `Makefile'.
  Prerequisite `../../backend.RecursiveMakeBackend.built' is older than target `Makefile'.
 No need to remake target `Makefile'.
Updating goal targets....
Considering target file `default'.

It's not apparent from the output, but I guess GNU make touches Makefile as part of self-reevaluation.
Summary: Touching a Makefile.in to add/remove a mochitest causes everything to be rebuilt → Makefile target invalidated after config.status if runnning with make -C
Assignee

Comment 9

6 years ago
Oh, this is from rules.mk, not a GNU make internal mechanism:

# Since Makefile is listed as a global dependency, this has the
# unfortunate side-effect of invalidating all targets if it is executed.
# So e.g. if you are in /dom/bindings and /foo/moz.build changes,
# /dom/bindings will get invalidated. The upside is if the current
# Makefile/backend.mk is updated as a result of backend regeneration, we
# actually pick up the changes. This should reduce the amount of
# required clobbers and is thus the lesser evil.
Makefile: $(DEPTH)/backend.RecursiveMakeBackend.built
	@$(TOUCH) $@
Assignee

Updated

6 years ago
Duplicate of this bug: 885727
Assignee

Comment 11

6 years ago
I consider the crux of this bug WONTFIX because I believe the current
mechanism for direct make invocation is the best we can get it. However,
I've provided this patch to extensively document behavior so it's not
spread out over half a dozen bugs (as it is now).

In order to make things better, we'll need to a) sacrifice correctness,
b) have people execute make with a wrapper that knows how to regenerate the build
backend, or c) go down a deep rabbit hole in the backend code (that may not
pan out).

I'm concerned "c" won't work and thus am ruling it out.

I'll put "a" on the table because it will make some developers happy.
However, it's a giant paper cut that I fear many will hit and will
inevitably result in developers filing bugs "I ran make but my changes
to Makefile.in or moz.build weren't picked up."

I think "b" is the long term solution and I'd like to move us in that
direction. (FWIW "b" is required for a Tup backend.) Regardless of what
we do here, we should file a bug so |mach build| knows about backend
regeneration.

Perhaps there is room for a compromise. Perhaps we could remove the
Makefile touch and add a warning message instead? Perhaps we could offer
an escape hatch by way of environment variable to disable the touch.
I'll entertain proposals.

tl;dr There is no perfect solution here. I wish there was, but there
isn't. This is an unfortunate regression due to the introduction of
moz.build files. We have to look forward and deal with it, I fear.
Attachment #766350 - Flags: review?(ted)
Attachment #766350 - Flags: review?(mh+mozilla)
Comment on attachment 766350 [details] [diff] [review]
Thoroughly document backend generation semantics

Review of attachment 766350 [details] [diff] [review]:
-----------------------------------------------------------------

Wait a minute. If backend.RecursiveMakeBackend.built is decided to be outdated because of a change in a moz.build, and config.status is re-run, and the Makefile has not changed, it means the moz.build change that triggered all that didn't change the Makefile. Now, it may change a backend.mk, and make doesn't handle that, but if you make the Makefile depend on backend.mk and backend.mk on backend.RecursiveMakeBackend.built instead, then things should work much better.

At least, it works with the small following testcase:
all:
	echo $@

Makefile: backend.mk
	touch $@

backend.mk: rebuilt
	:

rebuilt: moz.build
	touch $@ # config.status
	touch backend.mk

$(warning foo)

This would also work if config.status was made to touch Makefile when it updates the corresponding backend.mk. Then the following works:
all:
	echo $@

Makefile: rebuilt
	:

rebuilt: moz.build
	touch $@ # config.status
	touch backend.mk
	touch Makefile

$(warning foo)
Attachment #766350 - Flags: review?(ted)
Attachment #766350 - Flags: review?(mh+mozilla)
Attachment #766350 - Flags: review-
So here's what I think should happen

The rule at[0] exists because moz.build files can have "spooky action at a distance" effects.  We rerun config.status if any moz.build (or Makefile.in) has changed anywhere in the tree.  That updates the backend file and next time we run in a leaf directory the Makefile is touched which causes everything to rebuild.  This all exists to rerun things if you change foo/moz.build and then build in bar/moz.build and foo/moz.build affects what happens in bar/.  I think this is rare enough that we should just drop support for it.  If you do this you need to do a top-level rebuild, otherwise we just check the Makefile.in/moz.build in the current directory.  This avoids what effectively rebuilds the entire tree each time anything changes.

[0] http://hg.mozilla.org/mozilla-central/annotate/7ff96bd19c1c/config/rules.mk#l663
Assignee

Comment 14

6 years ago
The solution of having the build driver check backed update state is
the simplest. I believe this is what khuey proposed in the last comment.
This patch implements that. Please note that this will likely regress
build system behavior for people not building through mach. The two big
complaints about mach I've heard from people have been the timestamp
prefixing (fixed in bug 847175) and some lingering objdir detection
issues (vlad gave me enough info to track down the problem and I should
have a patch in bug 920679 shortly). Assuming bug 920679 fixes the
last environment detection issue, there should be no excuses left for
people to not build through mach. Even the power users (like bz) should
be able to adapt their build scripts to invoke mach instead of make.
Annoying, yes. But it should be a one-time transition. There are so many
wins from having builds go through a wrapper script. It's about time we
enforced it.

Anyway on to this patch.

Before, we checked if config.status was stale in any entrant Makefile
(top level or child directory). This had undesirable side-effects for
partial tree builds, notably that if the build backend was out of date,
the current Makefile was invalidated.

With this patch, we removed backend update checking from child
Makefiles. We now perform the backend update checked in the toplevel
Makefile only. Since we don't want to break partial tree builds, |mach
build| will call out to this make target when performing partial tree
builds. Since no file touching is involved, no entrant Makefile targets
are invalided.
Attachment #812033 - Flags: review?(mh+mozilla)
Comment on attachment 812033 [details] [diff] [review]
Change when build backend update check it performed

Review of attachment 812033 [details] [diff] [review]:
-----------------------------------------------------------------

How about adding a backend target at top-level, that would do this too, and a failure when one does make -C foo and backend files under foo/ are outdated? (saying to use make backend)

::: Makefile.in
@@ +62,5 @@
> +#
> +# The mach build driver will ensure the backend is up to date for partial tree
> +# builds. This cleanly avoids most of the pain.
> +
> +ifndef MOZBUILD_BACKEND_CHECKED

sounds like you can remove the ifndef and the export.
Attachment #812033 - Flags: review?(mh+mozilla) → feedback+
Assignee

Comment 16

6 years ago
Incorporated review feedback. Instead of adding yet another invisible
make target, I created a mach command. I figure we'll need to implement
this command when we support multiple build backends anyway. So we might
as well implement it now.
Attachment #812750 - Flags: review?(mh+mozilla)
Assignee

Updated

6 years ago
Attachment #812033 - Attachment is obsolete: true
Comment on attachment 812750 [details] [diff] [review]
Change when build backend update check it performed

Review of attachment 812750 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if you fix rules.mk to error out.

::: config/rules.mk
@@ +442,3 @@
>  define SUBMAKE # $(call SUBMAKE,target,directory,static)
>  +@$(UPDATE_TITLE)
> ++$(MAKE) $(if $(2),-C $(2)) $(1)

As a followup, you could remove the now useless third argument in SUBMAKE calls.

@@ +642,5 @@
>  else
>  HOST_OUTOPTION = -o # eol
>  endif
>  ################################################################################
>  

You should still add something like:

include $(DEPTH)/backend.RecursiveMakeBackend.built.pp

$(DEPTH)/backend.RecursiveMakeBackend.built.pp:
        $(error Build configuration changed. Run mach build-backend)

::: python/mozbuild/mozbuild/mach_commands.py
@@ +505,5 @@
> +        description='Generate a backend used to build the tree.')
> +    def build_backend(self):
> +        # When we support multiple build backends (Tup, Visual Studio, etc),
> +        # this command will be expanded to support choosing what to generate.
> +        # Until then, hardcode config.status.

fwiw, i think even in the future with multiple build backends it should still be config.status creating it.
Attachment #812750 - Flags: review?(mh+mozilla) → review+
Assignee

Comment 18

6 years ago
(In reply to Mike Hommey [:glandium] from comment #17)
> You should still add something like:
> 
> include $(DEPTH)/backend.RecursiveMakeBackend.built.pp
> 
> $(DEPTH)/backend.RecursiveMakeBackend.built.pp:
>         $(error Build configuration changed. Run mach build-backend)

This won't actually do anything since all backend.RecursiveMakeBackend.built* references have been removed from rules.mk and therefore they don't show up in the default target list.

We can't simply add a "Makefile: $(DEPTH)/backend.RecursiveMakeBackend.built" reference back in because if config.status is executed, there's no guarantee the local Makefile or backend.mk will be updated.

Checking if the backend is up to date is expensive - as make will need to stat() a few thousand moz.build, Makefile.in, and test manifest files. If we add checking of backend.RecursiveMakeBackend.built back into rules.mk, we'll need the MOZBUILD_BACKEND_CHECKED exported variable again. Is this what you're advocating? If so, the patch would effectively be preserving the existing rules for non-toplevel directories except we'd replace $(touch) Makefile and config.status execution with $(error)s.

I think the easiest way to handle things is to have rules.mk enforce that non-toplevel builds are using mach. e.g. ifndef MACH $(error). But that's been controversial and will surely draw ire. It will arguably happen eventually. Do we want to do that now or should we continue to wait? Do you have a better idea?
Assignee

Comment 19

6 years ago
Incorporated review feedback. Partial tree builds on an outdated build
config not building through mach will get a build error. Said error
message strongly encourages building through mach.
Attachment #821947 - Flags: review?(mh+mozilla)
Assignee

Updated

6 years ago
Attachment #766350 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #812750 - Attachment is obsolete: true
Comment on attachment 821947 [details] [diff] [review]
Change when build backend update check it performed

Review of attachment 821947 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile.in
@@ +66,5 @@
> +#
> +# The mach build driver will ensure the backend is up to date for partial tree
> +# builds. This cleanly avoids most of the pain.
> +
> +backend.RecursiveMakeBackend.built:

You've been bitrotted ;)

::: python/mozbuild/mozbuild/mach_commands.py
@@ +357,5 @@
>  
> +                # Ensure build backend is up to date. While there are rules in
> +                # rules.mk to do this for non-toplevel builds, the side-effects
> +                # are weird (it touches the invoked Makefile, invalidating most
> +                # targets).

rules.mk doesn't have rules for non-toplevel builds anymore, right?
Attachment #821947 - Flags: review?(mh+mozilla) → review+
Assignee

Comment 21

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0bf12c1d1d

With unbitrotting and fixed comment.
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/9d0bf12c1d1d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 933779

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.