1210642 - Use install manifests for preprocessed files in the FasterMake build backend

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: s/APP_BUILDID/MOZ_APP_BUILDID/ to use the same variable name across products

Comment 2

[Mike Hommey [:glandium]]

Attachment: Allow to pass defines overrides when processing install manifests

Comment 3

[Mike Hommey [:glandium]]

Attachment: Add support for --silence-missing-directive-warnings for preprocessing within install manifests

Comment 4

[Mike Hommey [:glandium]]

Attachment: Use install manifests for preprocessed files in the FasterMake build backend

Comment 5

[Gregory Szorc [:gps]]

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).

Comment 6

[Gregory Szorc [:gps]]

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.

Comment 7

[Gregory Szorc [:gps]]

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.

Comment 8

[Gregory Szorc [:gps]]

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.

Comment 9

[Mike Hommey [:glandium]]

(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.

Comment 10

[Mike Hommey [:glandium]]

Attachment: s/APP_BUILDID/MOZ_APP_BUILDID/ to use the same variable name across products

Gotcha. Carrying over r+

Comment 11

[Mike Hommey [:glandium]]

Attachment: Add support for --silence-missing-directive-warnings for preprocessing within install manifests

Comment 12

[Mike Hommey [:glandium]]

Attachment: Allow to pass defines overrides when processing install manifests

Carrying over r+

Comment 13

[Mike Hommey [:glandium]]

Attachment: Add support for --silence-missing-directive-warnings for preprocessing within install manifests

Comment 14

[Pulsebot]

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

[Carsten Book [:Tomcat]]

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