Closed Bug 1242663 Opened 4 years ago Closed 4 years ago

Removing a Makefile.in requires a clobber

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mshal, Assigned: mshal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

When removing a Makefile.in, an incremental build dies with:

INFO -  gmake[2]: *** No rule to make target `/builds/slave/m-in-l64-d-0000000000000000000/build/src/testing/mochitest/manifests/Makefile.in', needed by `backend.RecursiveMakeBackend'.  Stop.

The problem is that the backend dependencies were changed in bug 1239217, and this no longer includes the stub targets for handling file removal.
This seems to fix it while testing locally. Is there any reason we would need to add a separate line for each file rather than putting them all in one rule? eg:

foo/Makefile.in:
bar/Makefile.in:

vs:

foo/Makefile.in bar/Makefile.in:

(This patch does the latter)
Assignee: nobody → mshal
Attachment #8711810 - Flags: review?(mh+mozilla)
Comment on attachment 8711810 [details] [diff] [review]
0001-Bug-1242663-Add-targets-for-each-backend-dependency.patch

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

::: Makefile.in
@@ +114,5 @@
>  	@$(TOUCH) $@
>  
>  define build_backend_rule
>  $(1): $$(shell cat $(1).in)
> +$$(shell cat $(1).in):

That makes us spawn a shell to read the file twice. Use a "temporary" variable?
Attachment #8711810 - Flags: review?(mh+mozilla)
Ahh, good call.
Attachment #8711810 - Attachment is obsolete: true
Attachment #8712199 - Flags: review?(mh+mozilla)
Comment on attachment 8712199 [details] [diff] [review]
0001-Bug-1242663-Add-targets-for-each-backend-dependency.patch

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

Stealing because I got bitten by this trying to land a patch today.

::: Makefile.in
@@ +113,5 @@
>  Makefile: $(BUILD_BACKEND_FILES)
>  	@$(TOUCH) $@
>  
>  define build_backend_rule
> +files := $$(shell cat $(1).in)

This might want to be $(1)_files := ...

because we might be evaluating build_backend_rule more than once.
Attachment #8712199 - Flags: review?(mh+mozilla) → review+
Updated to use $(1)_files, r+ carried forward.

Note that we do currently have multiple backends (recursive make & faster make).  The use of 'files' in the previous patch is expanded during the eval, so each backend is evaluated with its own set of files. However, using a separate variable for each is much more clear when dumping the database with 'make -p', since you can see the list of files retrieved for each backend, rather than just the last backend.
Attachment #8712199 - Attachment is obsolete: true
Attachment #8712386 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f8fc7a05f96b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.