Closed
Bug 1445398
Opened 6 years ago
Closed 6 years ago
Do not regenerate buildid.h for every tup build
Categories
(Firefox Build System :: General, enhancement)
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: chmanchester, Assigned: chmanchester)
References
Details
Attachments
(1 file)
We need to bring the fix for bug 1298328 to tup.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → cmanchester
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8961121 [details] Bug 1445398 - Do not re-generate buildid.h for every Tup build. https://reviewboard.mozilla.org/r/229922/#review235854 ::: python/mozbuild/mozbuild/backend/tup.py:191 (Diff revision 1) > > # These are 'group' dependencies - All rules that list these as an output > # will be built before any rules that list this as an input. > self._installed_idls = '$(MOZ_OBJ_ROOT)/<installed-idls>' > self._installed_files = '$(MOZ_OBJ_ROOT)/<installed-files>' > + # The preprocessor can hide dependencies from Tup in some cases (when The problem here is that tup complains about the missing dependency on buildid.h / source-repo.h when they are #included by application.ini.h, right? If so I think we can just change the appini.inputs in build/moz.build to have '!../buildid.h' and '!../source-repo.h', and then we don't need a separate early-generated-files group. ::: python/mozbuild/mozbuild/backend/tup.py:518 (Diff revision 1) > # '%%' in the Tupfile. > marker = '%%' if target.endswith('.css') else '#' > > cmd = self._py_action('preprocessor') > cmd.extend([shell_quote(d) for d in backend_file.defines]) > - cmd.extend(['$(ACDEFINES)', '%f', '-o', '%o', '--marker=%s' % marker]) > + cmd.extend(['$(ACDEFINES)', input_file, '-o', '%o', '--marker=%s' % marker]) You could keep the '%f' here and instead have self._early_generated_files in extra_inputs. The extra_inputs do not show up in %f while inputs do. (Similarly for extra_outputs and outputs in %o).
Attachment #8961121 -
Flags: review?(mshal)
Assignee | ||
Comment 3•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8961121 [details] Bug 1445398 - Do not re-generate buildid.h for every Tup build. https://reviewboard.mozilla.org/r/229922/#review235854 > The problem here is that tup complains about the missing dependency on buildid.h / source-repo.h when they are #included by application.ini.h, right? If so I think we can just change the appini.inputs in build/moz.build to have '!../buildid.h' and '!../source-repo.h', and then we don't need a separate early-generated-files group. It's an issue for application.ini as well as some other preprocessed files that include these files. Unfortunately adding these headers as inputs doesn't quite work because it also feeds them to the preprocessor (and we don't have a facility to do something like this for regular preprocessed files). > You could keep the '%f' here and instead have self._early_generated_files in extra_inputs. The extra_inputs do not show up in %f while inputs do. (Similarly for extra_outputs and outputs in %o). Thanks for the tip, will do!
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
(In reply to Chris Manchester (:chmanchester) from comment #3) > Comment on attachment 8961121 [details] > Bug 1445398 - Do not re-generate buildid.h for every Tup build. > > https://reviewboard.mozilla.org/r/229922/#review235854 > > > The problem here is that tup complains about the missing dependency on buildid.h / source-repo.h when they are #included by application.ini.h, right? If so I think we can just change the appini.inputs in build/moz.build to have '!../buildid.h' and '!../source-repo.h', and then we don't need a separate early-generated-files group. > > It's an issue for application.ini as well as some other preprocessed files > that include these files. Unfortunately adding these headers as inputs > doesn't quite work because it also feeds them to the preprocessor (and we > don't have a facility to do something like this for regular preprocessed > files). Oh, looks like platform.ini is the other one? I guess we could call preprocessor.py for that as a GENERATED_FILES so we can specify buildid.h / source-repo.h as inputs similar to application.ini instead of having it as a FINAL_TARGET_PP_FILES. We can always revisit that later though - no need to block on this.
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8961121 [details] Bug 1445398 - Do not re-generate buildid.h for every Tup build. https://reviewboard.mozilla.org/r/229922/#review235942 Looks good! Please just update the comment before landing. ::: python/mozbuild/mozbuild/backend/tup.py:191 (Diff revision 2) > > # These are 'group' dependencies - All rules that list these as an output > # will be built before any rules that list this as an input. > self._installed_idls = '$(MOZ_OBJ_ROOT)/<installed-idls>' > self._installed_files = '$(MOZ_OBJ_ROOT)/<installed-files>' > + # The preprocessor can hide dependencies from Tup in some cases (when This comment confused me at first - it's not really that the preprocessor is hiding dependencies from tup, it's that the dependencies on those files aren't specified anywhere in moz.build. Tup does see the dependencies, and complains that it wasn't told about them ahead of time since those two files are generated from other rules. When I first read it I thought somehow tup wasn't picking up those dependencies and failing to re-build application.ini and such when buildid.h changes.
Attachment #8961121 -
Flags: review?(mshal) → review+
Comment hidden (mozreview-request) |
Pushed by cmanchester@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d2fdb62d06d0 Do not re-generate buildid.h for every Tup build. r=mshal
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d2fdb62d06d0
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•5 years ago
|
Version: Version 3 → 3 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•