Closed
Bug 1229241
Opened 9 years ago
Closed 9 years ago
Transform many moz.build variables to proxies to FINAL_TARGET{,_PP}_FILES
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox45 fixed)
RESOLVED
FIXED
mozilla45
| Tracking | Status | |
|---|---|---|
| firefox45 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(8 files)
|
1.67 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
|
1.44 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
|
5.18 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
|
20.09 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
|
7.88 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
|
18.94 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
|
17.80 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
|
12.22 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
While it is nice that moz.build have useful separate variables for various kinds of files in a Firefox build, it is less nice when writing build backends, and adds needless overhead for people writing new backends. OTOH, many of those variables are not saying much more than "install this there" or "preprocess this and put it there", which we already have "generic" variables for (FINAL_TARGET_FILES and FINAL_TARGET_PP_FILES), and "generic" mozbuild.frontend.data types for (FinalTargetFiles and FinalTargetPreprocessedFiles). The distinction between the various mozbuild.frontend.data types is not really interesting for backends, and it's just much simpler to make the magic happen at moz.build processing time rather than trying to create FinalTarget*Files objects in mozbuild.frontend.emitter.
|
Assignee | |
Updated•9 years ago
|
Summary: Transform many variables to proxies for FINAL_TARGET{,_PP}_FILES → Transform many moz.build variables to proxies to FINAL_TARGET{,_PP}_FILES
|
|
Assignee | |
Comment 1•9 years ago
|
||
This will allow a new kind of special variable where it is possible to do FOO += ['bar'] All the current special variables are either strings (for which __setitem__ would be called with a different string object), or a read-only dict (which doesn't allow modifications).
Attachment #8693915 -
Flags: review?(gps)
|
|
Assignee | |
Comment 2•9 years ago
|
||
Attachment #8693916 -
Flags: review?(gps)
|
|
Assignee | |
Comment 3•9 years ago
|
||
Attachment #8693917 -
Flags: review?(gps)
|
|
Assignee | |
Comment 4•9 years ago
|
||
Attachment #8693919 -
Flags: review?(gps)
|
|
Assignee | |
Comment 5•9 years ago
|
||
Attachment #8693920 -
Flags: review?(gps)
|
|
Assignee | |
Comment 6•9 years ago
|
||
Attachment #8693921 -
Flags: review?(gps)
|
|
Assignee | |
Comment 7•9 years ago
|
||
Attachment #8693923 -
Flags: review?(gps)
|
|
Assignee | |
Comment 8•9 years ago
|
||
Attachment #8693924 -
Flags: review?(gps)
|
||
Comment 9•9 years ago
|
||
Comment on attachment 8693915 [details] [diff] [review] Allow moz.build special variables to be set, as long as the value is not modified Review of attachment 8693915 [details] [diff] [review]: ----------------------------------------------------------------- Sure. Using "is" makes this pretty safe.
Attachment #8693915 -
Flags: review?(gps) → review+
|
|
||
Comment 10•9 years ago
|
||
Comment on attachment 8693916 [details] [diff] [review] Reject FINAL_TARGET{,_PP}_FILES along DIST_INSTALL = False Review of attachment 8693916 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +694,5 @@ > if not all_files: > continue > + if dist_install is False: > + raise SandboxValidationError( > + '%s makes no sense along DIST_INSTALL = False' % var, %s cannot be used with DIST_INSTALL = False.
Attachment #8693916 -
Flags: review?(gps) → review+
|
|
||
Comment 11•9 years ago
|
||
Comment on attachment 8693917 [details] [diff] [review] Support merging HierarchicalStringLists Review of attachment 8693917 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/reader.py @@ +415,5 @@ > # The sandbox will do all the necessary checks for these merges. > for key, value in context.items(): > if isinstance(value, dict): > self[key].update(value) > + elif isinstance(value, (list, HierarchicalStringList)): I think I see where this is going...
Attachment #8693917 -
Flags: review?(gps) → review+
|
|
||
Comment 12•9 years ago
|
||
Comment on attachment 8693919 [details] [diff] [review] Redirect EXTRA{_PP}_COMPONENTS to FINAL_TARGET{,_PP}_FILES.components Review of attachment 8693919 [details] [diff] [review]: ----------------------------------------------------------------- This is extremely nice! ::: python/mozbuild/mozbuild/frontend/context.py @@ +1955,5 @@ > > Access to an unknown variable will return None. > """), > + > + 'EXTRA_COMPONENTS': (lambda context: context['FINAL_TARGET_FILES'].components._strings, list, I wonder if it's worth killing EXTRA_COMPONENTS and/or renaming FINAL_TARGET_FILES to something more obvious.
Attachment #8693919 -
Flags: review?(gps) → review+
|
|
||
Comment 13•9 years ago
|
||
Comment on attachment 8693920 [details] [diff] [review] Differentiate preprocessed and non-preprocessed JS pref files Review of attachment 8693920 [details] [diff] [review]: ----------------------------------------------------------------- I'm guessing a future patch will actually change processing behavior...
Attachment #8693920 -
Flags: review?(gps) → review+
|
|
||
Comment 14•9 years ago
|
||
Comment on attachment 8693921 [details] [diff] [review] Use FINAL_TARGET{,_PP}_FILES for JS_PREFERENCE{,_PP}_FILES Review of attachment 8693921 [details] [diff] [review]: ----------------------------------------------------------------- Love it. ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +697,5 @@ > 'File listed in %s does not exist: %s' > % (var, f), context) > > + if has_prefs and (context.get('XPI_NAME') or > + context.get('DIST_SUBDIR')): This probably warrants an inline comment.
Attachment #8693921 -
Flags: review?(gps) → review+
|
|
||
Comment 15•9 years ago
|
||
Comment on attachment 8693923 [details] [diff] [review] Redirect RESOURCE_FILES to FINAL_TARGET_FILES.res Review of attachment 8693923 [details] [diff] [review]: ----------------------------------------------------------------- This is beautiful. ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +698,5 @@ > > + if has_resources and (context.get('DIST_SUBDIR') or > + context.get('XPI_NAME')): > + raise SandboxValidationError( > + 'RESOURCES_FILES is not allowed along DIST_SUBDIR or ' Please fixup grammar similar to my other suggestions.
Attachment #8693923 -
Flags: review?(gps) → review+
|
|
||
Comment 16•9 years ago
|
||
Comment on attachment 8693924 [details] [diff] [review] Use FINAL_TARGET{,_PP}_FILES for EXTRA{,_PP}_JS_MODULES Review of attachment 8693924 [details] [diff] [review]: ----------------------------------------------------------------- Wow! I'm kinda curious how many lines/targets we removed from backend.mk files with this refactor. It's got to be in the thousands.
Attachment #8693924 -
Flags: review?(gps) → review+
|
|
Assignee | |
Comment 17•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #16) > Comment on attachment 8693924 [details] [diff] [review] > Use FINAL_TARGET{,_PP}_FILES for EXTRA{,_PP}_JS_MODULES > > Review of attachment 8693924 [details] [diff] [review]: > ----------------------------------------------------------------- > > Wow! I'm kinda curious how many lines/targets we removed from backend.mk > files with this refactor. It's got to be in the thousands. On my machine: _build_manifests/install/dist_bin | 692 +++++++++++++++++++++ accessible/jsat/backend.mk | 5 addon-sdk/backend.mk | 365 ----------- browser/components/backend.mk | 5 browser/components/customizableui/backend.mk | 16 browser/components/downloads/backend.mk | 5 browser/components/migration/backend.mk | 8 browser/components/newtab/backend.mk | 5 browser/components/places/backend.mk | 5 browser/components/pocket/backend.mk | 5 browser/components/sessionstore/backend.mk | 5 browser/components/tabview/backend.mk | 5 browser/components/translation/backend.mk | 5 browser/components/uitour/backend.mk | 5 browser/experiments/backend.mk | 5 browser/modules/backend.mk | 13 devtools/client/shims/backend.mk | 10 devtools/server/shims/backend.mk | 5 devtools/shared/shims/backend.mk | 15 dom/activities/backend.mk | 5 dom/alarm/backend.mk | 5 dom/apps/backend.mk | 18 dom/base/backend.mk | 5 dom/browser-element/backend.mk | 5 dom/contacts/backend.mk | 5 dom/datastore/backend.mk | 5 dom/inputmethod/backend.mk | 5 dom/manifest/backend.mk | 5 dom/media/backend.mk | 5 dom/media/webvtt/backend.mk | 5 dom/messages/backend.mk | 5 dom/notification/backend.mk | 5 dom/payment/backend.mk | 8 dom/permission/backend.mk | 5 dom/phonenumberutils/backend.mk | 13 dom/presentation/backend.mk | 5 dom/push/backend.mk | 5 dom/requestsync/backend.mk | 5 dom/resourcestats/backend.mk | 5 dom/settings/backend.mk | 5 dom/system/backend.mk | 5 editor/backend.mk | 5 intl/locale/backend.mk | 5 js/ductwork/debugger/backend.mk | 5 js/xpconnect/loader/backend.mk | 5 netwerk/base/backend.mk | 5 netwerk/protocol/http/backend.mk | 5 root-deps.mk | 76 -- root.mk | 4 services/cloudsync/backend.mk | 5 services/common/backend.mk | 16 services/crypto/backend.mk | 13 services/datareporting/backend.mk | 8 services/fxaccounts/backend.mk | 13 services/healthreport/backend.mk | 18 services/metrics/backend.mk | 18 services/sync/backend.mk | 23 toolkit/components/addoncompat/backend.mk | 5 toolkit/components/asyncshutdown/backend.mk | 5 toolkit/components/contentprefs/backend.mk | 5 toolkit/components/crashes/backend.mk | 5 toolkit/components/crashmonitor/backend.mk | 5 toolkit/components/ctypes/backend.mk | 5 toolkit/components/extensions/backend.mk | 5 toolkit/components/formautofill/backend.mk | 5 toolkit/components/jsdownloads/src/backend.mk | 14 toolkit/components/lz4/backend.mk | 5 toolkit/components/microformats/backend.mk | 5 toolkit/components/osfile/backend.mk | 8 toolkit/components/osfile/modules/backend.mk | 5 toolkit/components/passwordmgr/backend.mk | 13 toolkit/components/perf/backend.mk | 5 toolkit/components/perfmonitoring/backend.mk | 5 toolkit/components/places/backend.mk | 5 toolkit/components/promiseworker/backend.mk | 5 toolkit/components/promiseworker/worker/backend.mk | 5 toolkit/components/prompts/src/backend.mk | 5 toolkit/components/reader/backend.mk | 10 toolkit/components/reflect/backend.mk | 5 toolkit/components/satchel/backend.mk | 13 toolkit/components/search/backend.mk | 5 toolkit/components/social/backend.mk | 5 toolkit/components/sqlite/backend.mk | 5 toolkit/components/telemetry/backend.mk | 5 toolkit/components/thumbnails/backend.mk | 5 toolkit/components/url-classifier/backend.mk | 5 toolkit/components/viewsource/backend.mk | 5 toolkit/components/workerloader/backend.mk | 5 toolkit/crashreporter/backend.mk | 5 toolkit/forgetaboutsite/backend.mk | 5 toolkit/identity/backend.mk | 13 toolkit/modules/backend.mk | 14 toolkit/mozapps/downloads/backend.mk | 13 toolkit/mozapps/extensions/backend.mk | 13 toolkit/mozapps/extensions/internal/backend.mk | 14 toolkit/mozapps/update/backend.mk | 5 toolkit/webapps/backend.mk | 9 tools/profiler/backend.mk | 5 webapprt/backend.mk | 5 99 files changed, 819 insertions(+), 1007 deletions(-)
|
|
Assignee | |
Comment 18•9 years ago
|
||
Actually that's only for the last patch. For the entire patchset from this bug: _build_manifests/install/dist_bin | 911 +++++++++++++++++++++ _build_manifests/install/dist_xpi-stage | 4 _build_manifests/install/tests | 1 accessible/jsat/backend.mk | 5 addon-sdk/backend.mk | 365 -------- browser/app/backend.mk | 4 browser/branding/unofficial/backend.mk | 1 browser/components/backend.mk | 14 browser/components/customizableui/backend.mk | 16 browser/components/downloads/backend.mk | 5 browser/components/feeds/backend.mk | 9 browser/components/migration/backend.mk | 18 browser/components/newtab/backend.mk | 5 browser/components/places/backend.mk | 5 browser/components/pocket/backend.mk | 5 browser/components/selfsupport/backend.mk | 4 browser/components/sessionstore/backend.mk | 10 browser/components/shell/backend.mk | 4 browser/components/tabview/backend.mk | 5 browser/components/translation/backend.mk | 5 browser/components/uitour/backend.mk | 5 browser/experiments/backend.mk | 9 browser/fuel/backend.mk | 7 browser/modules/backend.mk | 19 devtools/client/backend.mk | 4 devtools/client/preferences/backend.mk | 4 devtools/client/shims/backend.mk | 10 devtools/client/webide/backend.mk | 4 devtools/client/webide/components/backend.mk | 4 devtools/server/shims/backend.mk | 5 devtools/shared/shims/backend.mk | 15 dom/activities/backend.mk | 12 dom/alarm/backend.mk | 9 dom/apps/backend.mk | 32 dom/base/backend.mk | 25 dom/bindings/test/backend.mk | 5 dom/browser-element/backend.mk | 13 dom/contacts/backend.mk | 9 dom/datastore/backend.mk | 9 dom/html/backend.mk | 4 dom/inputmethod/backend.mk | 9 dom/manifest/backend.mk | 5 dom/media/backend.mk | 9 dom/media/webvtt/backend.mk | 9 dom/messages/backend.mk | 11 dom/newapps/backend.mk | 4 dom/notification/backend.mk | 13 dom/payment/backend.mk | 17 dom/permission/backend.mk | 13 dom/phonenumberutils/backend.mk | 17 dom/presentation/backend.mk | 9 dom/presentation/provider/backend.mk | 4 dom/push/backend.mk | 11 dom/requestsync/backend.mk | 10 dom/resourcestats/backend.mk | 9 dom/secureelement/backend.mk | 4 dom/settings/backend.mk | 12 dom/system/backend.mk | 13 dom/tv/backend.mk | 4 dom/workers/test/extensions/traditional/backend.mk | 4 dom/xslt/xslt/backend.mk | 4 editor/backend.mk | 5 intl/locale/backend.mk | 5 js/ductwork/debugger/backend.mk | 5 js/xpconnect/loader/backend.mk | 5 js/xpconnect/tests/components/native/backend.mk | 3 layout/style/backend.mk | 4 layout/tools/recording/backend.mk | 4 layout/tools/reftest/backend.mk | 7 netwerk/base/backend.mk | 9 netwerk/protocol/http/backend.mk | 9 netwerk/test/httpserver/backend.mk | 4 root-deps.mk | 91 -- root.mk | 4 services/cloudsync/backend.mk | 5 services/common/backend.mk | 20 services/crypto/backend.mk | 16 services/datareporting/backend.mk | 15 services/fxaccounts/backend.mk | 13 services/healthreport/backend.mk | 24 services/metrics/backend.mk | 18 services/sync/backend.mk | 31 startupcache/test/backend.mk | 4 testing/marionette/components/backend.mk | 7 toolkit/components/addoncompat/backend.mk | 10 toolkit/components/asyncshutdown/backend.mk | 9 toolkit/components/backend.mk | 4 toolkit/components/captivedetect/backend.mk | 4 toolkit/components/console/backend.mk | 4 toolkit/components/contentprefs/backend.mk | 9 toolkit/components/crashes/backend.mk | 9 toolkit/components/crashmonitor/backend.mk | 9 toolkit/components/ctypes/backend.mk | 5 toolkit/components/downloads/backend.mk | 4 toolkit/components/extensions/backend.mk | 5 toolkit/components/feeds/backend.mk | 4 toolkit/components/filepicker/backend.mk | 7 toolkit/components/formautofill/backend.mk | 13 toolkit/components/jsdownloads/src/backend.mk | 18 toolkit/components/lz4/backend.mk | 5 toolkit/components/microformats/backend.mk | 5 toolkit/components/osfile/backend.mk | 8 toolkit/components/osfile/modules/backend.mk | 5 toolkit/components/passwordmgr/backend.mk | 25 toolkit/components/perf/backend.mk | 5 toolkit/components/perfmonitoring/backend.mk | 5 toolkit/components/places/backend.mk | 21 toolkit/components/privatebrowsing/backend.mk | 4 toolkit/components/processsingleton/backend.mk | 5 toolkit/components/promiseworker/backend.mk | 5 toolkit/components/promiseworker/worker/backend.mk | 5 toolkit/components/prompts/src/backend.mk | 9 toolkit/components/reader/backend.mk | 10 toolkit/components/reflect/backend.mk | 5 toolkit/components/satchel/backend.mk | 23 toolkit/components/search/backend.mk | 15 toolkit/components/social/backend.mk | 5 toolkit/components/sqlite/backend.mk | 5 toolkit/components/telemetry/backend.mk | 9 toolkit/components/terminator/backend.mk | 4 toolkit/components/thumbnails/backend.mk | 9 toolkit/components/timermanager/backend.mk | 4 toolkit/components/url-classifier/backend.mk | 15 toolkit/components/urlformatter/backend.mk | 7 toolkit/components/utils/backend.mk | 4 toolkit/components/viewsource/backend.mk | 5 toolkit/components/workerloader/backend.mk | 5 toolkit/components/xulstore/backend.mk | 4 toolkit/crashreporter/backend.mk | 5 toolkit/forgetaboutsite/backend.mk | 5 toolkit/identity/backend.mk | 13 toolkit/modules/backend.mk | 14 toolkit/mozapps/downloads/backend.mk | 20 toolkit/mozapps/extensions/backend.mk | 25 toolkit/mozapps/extensions/internal/backend.mk | 14 toolkit/mozapps/handling/backend.mk | 4 toolkit/mozapps/update/backend.mk | 10 toolkit/pluginproblem/backend.mk | 3 toolkit/webapps/backend.mk | 9 tools/profiler/backend.mk | 5 tools/quitter/backend.mk | 1 uriloader/exthandler/backend.mk | 8 webapprt/backend.mk | 16 xpcom/ds/backend.mk | 4 144 files changed, 1340 insertions(+), 1265 deletions(-)
|
|
Assignee | |
Comment 19•9 years ago
|
||
Presumably, this is due to EXTRA_PP_COMPONENTS adding lines to backend.mk.
|
||
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b69aaa0c3711 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b183b8a6116 https://hg.mozilla.org/integration/mozilla-inbound/rev/63bfa84f09aa https://hg.mozilla.org/integration/mozilla-inbound/rev/88981a083ec9 https://hg.mozilla.org/integration/mozilla-inbound/rev/81d63799328b https://hg.mozilla.org/integration/mozilla-inbound/rev/839564f83b49 https://hg.mozilla.org/integration/mozilla-inbound/rev/48d86aacdfc5 https://hg.mozilla.org/integration/mozilla-inbound/rev/192e590b1a63 https://hg.mozilla.org/integration/mozilla-inbound/rev/60c1ec5f8369
|
|
Assignee | |
Comment 21•9 years ago
|
||
(In reply to Pulsebot from comment #20) > https://hg.mozilla.org/integration/mozilla-inbound/rev/839564f83b49 Note this one is a trivial change that I r=me'ed to fix mulet builds. I will file a separate bug to fix the underlying reason this was necessary, which is that FINAL_TARGET_FILES is not using SourcePaths, and that JS_PREFERENCE_FILES had a manual treatment for paths starting with /, which those were the only occurrence of.
|
||
Comment 22•9 years ago
|
||
Aw yiss.
|
||
Comment 23•9 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/b69aaa0c3711 https://hg.mozilla.org/mozilla-central/rev/7b183b8a6116 https://hg.mozilla.org/mozilla-central/rev/63bfa84f09aa https://hg.mozilla.org/mozilla-central/rev/88981a083ec9 https://hg.mozilla.org/mozilla-central/rev/81d63799328b https://hg.mozilla.org/mozilla-central/rev/839564f83b49 https://hg.mozilla.org/mozilla-central/rev/48d86aacdfc5 https://hg.mozilla.org/mozilla-central/rev/192e590b1a63 https://hg.mozilla.org/mozilla-central/rev/60c1ec5f8369
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1198013
|
||
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
•