Closed
Bug 1210642
Opened 9 years ago
Closed 9 years ago
Use install manifests for preprocessed files in the FasterMake build backend
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox44 fixed)
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(4 files, 4 obsolete files)
15.64 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
7.40 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
5.76 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
5.35 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8668742 -
Flags: review?(gps)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8668743 -
Flags: review?(gps)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8668744 -
Flags: review?(gps)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8668745 -
Flags: review?(gps)
Comment 5•9 years ago
|
||
Comment on attachment 8668742 [details] [diff] [review] s/APP_BUILDID/MOZ_APP_BUILDID/ to use the same variable name across products Review of attachment 8668742 [details] [diff] [review]: ----------------------------------------------------------------- There are a few references to "APP_BUILDID" still in the tree. Notably mobile/android/base/AppConstants.java.in and a separate, unrelated variable in testing/taskcluster/scripts/builder/build-simulator.sh. Probably worth consolidating to MOZ_APP_BUILDID for consistency (in at least the first case anyway).
Attachment #8668742 -
Flags: review?(gps) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8668743 [details] [diff] [review] Allow to pass defines overrides when processing install manifests Review of attachment 8668743 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/action/process_install_manifest.py @@ +23,5 @@ > for path in paths: > manifest |= InstallManifest(path=path) > > copier = FileCopier() > + manifest.populate_registry(copier, defines) Please use a named argument for override_defines. @@ +41,5 @@ > + name, value = values[0], 1 > + else: > + name, value = values > + if value.isdigit(): > + value = int(value) It feels a bit strange to be using mixed ints and strings here: I would think the preprocessor would be all string based! However, I do see there are int casts in the preprocessor. I guess this is needed for things beyond simple substitution. Still, I can't help but feel this is something that should be in the preprocessor. Meh. Probably scope bloat.
Attachment #8668743 -
Flags: review?(gps) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8668744 [details] [diff] [review] Add support for --silence-missing-directive-warnings for preprocessing within install manifests Review of attachment 8668744 [details] [diff] [review]: ----------------------------------------------------------------- Please go with the simpler data format. ::: python/mozbuild/mozpack/manifests.py @@ +298,5 @@ > + marker, > + self._encode_field_entry(defines), > + ] > + if silence_missing_directive_warnings: > + entry.append('1') The problem with this approach is once we add another field, we don't know whether entry[5] is a missing directive value or something else and we need to change the data format yet again. I would say always write (and read) all fields, even if there is a slight data overhead. We shouldn't need to worry reading files produced with the old format because changing the mtime of this file will cause moz.build to re-run and regenerate the install manifests.
Attachment #8668744 -
Flags: review?(gps)
Comment 8•9 years ago
|
||
Comment on attachment 8668745 [details] [diff] [review] Use install manifests for preprocessed files in the FasterMake build backend Review of attachment 8668745 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/faster/rules.mk @@ +92,2 @@ > -DAB_CD=en-US \ > + -DMOZ_APP_BUILDID=$(shell cat $(TOPOBJDIR)/config/buildid) \ I really wish we were grabbing the build id from a lazily defined variable in config.mk or something so the whole world doesn't have to code $(shell) calls. This would enable us to more easily do things like define the variable at the beginning of the build and export it to all Makefiles.
Attachment #8668745 -
Flags: review?(gps) → review+
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #5) > Comment on attachment 8668742 [details] [diff] [review] > s/APP_BUILDID/MOZ_APP_BUILDID/ to use the same variable name across products > > Review of attachment 8668742 [details] [diff] [review]: > ----------------------------------------------------------------- > > There are a few references to "APP_BUILDID" still in the tree. Notably > mobile/android/base/AppConstants.java.in O_o there's a MOZ_APP_BUILDID there in my tree.
Assignee | ||
Comment 10•9 years ago
|
||
Gotcha. Carrying over r+
Attachment #8668742 -
Attachment is obsolete: true
Attachment #8669205 -
Flags: review+
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8668744 -
Attachment is obsolete: true
Attachment #8669207 -
Flags: review?(gps)
Assignee | ||
Comment 12•9 years ago
|
||
Carrying over r+
Attachment #8668743 -
Attachment is obsolete: true
Attachment #8669208 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8669207 -
Attachment is obsolete: true
Attachment #8669207 -
Flags: review?(gps)
Attachment #8669229 -
Flags: review?(gps)
Updated•9 years ago
|
Attachment #8669229 -
Flags: review?(gps) → review+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f0ba759efbc https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fdb9d0bc08 https://hg.mozilla.org/integration/mozilla-inbound/rev/42199c7b225a https://hg.mozilla.org/integration/mozilla-inbound/rev/af55d7cf2093
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1f0ba759efbc https://hg.mozilla.org/mozilla-central/rev/f2fdb9d0bc08 https://hg.mozilla.org/mozilla-central/rev/42199c7b225a https://hg.mozilla.org/mozilla-central/rev/af55d7cf2093
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•