Closed
Bug 1242663
Opened 9 years ago
Closed 9 years ago
Removing a Makefile.in requires a clobber
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
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)
(deleted),
patch
|
mshal
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Ahh, good call.
Attachment #8711810 -
Attachment is obsolete: true
Attachment #8712199 -
Flags: review?(mh+mozilla)
Comment 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
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+
Comment 7•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•