Use install manifests for preprocessed files in the FasterMake build backend

RESOLVED FIXED in Firefox 44

Status

RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla44
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8668742 [details] [diff] [review]
s/APP_BUILDID/MOZ_APP_BUILDID/ to use the same variable name across products
Attachment #8668742 - Flags: review?(gps)
(Assignee)

Comment 2

3 years ago
Created attachment 8668743 [details] [diff] [review]
Allow to pass defines overrides when processing install manifests
Attachment #8668743 - Flags: review?(gps)
(Assignee)

Comment 3

3 years ago
Created attachment 8668744 [details] [diff] [review]
Add support for --silence-missing-directive-warnings for preprocessing within install manifests
Attachment #8668744 - Flags: review?(gps)
(Assignee)

Comment 4

3 years ago
Created attachment 8668745 [details] [diff] [review]
Use install manifests for preprocessed files in the FasterMake build backend
Attachment #8668745 - Flags: review?(gps)
(Assignee)

Updated

3 years ago
Blocks: 1210687

Comment 5

3 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

3 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

3 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

3 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

3 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

3 years ago
Created attachment 8669205 [details] [diff] [review]
s/APP_BUILDID/MOZ_APP_BUILDID/ to use the same variable name across products

Gotcha. Carrying over r+
Attachment #8668742 - Attachment is obsolete: true
Attachment #8669205 - Flags: review+
(Assignee)

Comment 11

3 years ago
Created attachment 8669207 [details] [diff] [review]
Add support for --silence-missing-directive-warnings for preprocessing within install manifests
Attachment #8668744 - Attachment is obsolete: true
Attachment #8669207 - Flags: review?(gps)
(Assignee)

Comment 12

3 years ago
Created attachment 8669208 [details] [diff] [review]
Allow to pass defines overrides when processing install manifests

Carrying over r+
Attachment #8668743 - Attachment is obsolete: true
Attachment #8669208 - Flags: review+
(Assignee)

Comment 13

3 years ago
Created attachment 8669229 [details] [diff] [review]
Add support for --silence-missing-directive-warnings for preprocessing within install manifests
Attachment #8669207 - Attachment is obsolete: true
Attachment #8669207 - Flags: review?(gps)
Attachment #8669229 - Flags: review?(gps)

Updated

3 years ago
Attachment #8669229 - Flags: review?(gps) → review+

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.