Closed Bug 1229241 Opened 8 years ago Closed 8 years ago

Transform many moz.build variables to proxies to FINAL_TARGET{,_PP}_FILES

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox45 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(8 files)

Allow moz.build special variables to be set, as long as the value is not modified
1.67 KB, patch
gps
: review+
Details | Diff | Splinter Review
Reject FINAL_TARGET{,_PP}_FILES along DIST_INSTALL = False
1.44 KB, patch
gps
: review+
Details | Diff | Splinter Review
Support merging HierarchicalStringLists
5.18 KB, patch
gps
: review+
Details | Diff | Splinter Review
Redirect EXTRA{_PP}_COMPONENTS to FINAL_TARGET{,_PP}_FILES.components
20.09 KB, patch
gps
: review+
Details | Diff | Splinter Review
Differentiate preprocessed and non-preprocessed JS pref files
7.88 KB, patch
gps
: review+
Details | Diff | Splinter Review
Use FINAL_TARGET{,_PP}_FILES for JS_PREFERENCE{,_PP}_FILES
18.94 KB, patch
gps
: review+
Details | Diff | Splinter Review
Redirect RESOURCE_FILES to FINAL_TARGET_FILES.res
17.80 KB, patch
gps
: review+
Details | Diff | Splinter Review
Use FINAL_TARGET{,_PP}_FILES for EXTRA{,_PP}_JS_MODULES
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.
Summary: Transform many variables to proxies for FINAL_TARGET{,_PP}_FILES → Transform many moz.build variables to proxies to FINAL_TARGET{,_PP}_FILES
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)
Depends on: 1229226
Blocks: 1229245
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 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 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 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 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 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 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 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+
(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(-)
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(-)
Presumably, this is due to EXTRA_PP_COMPONENTS adding lines to backend.mk.
(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.
Depends on: 1229279
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.