968245 - mozinfo.json should be regenerated if mozinfo.py changes

Status: NEW

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

I just added a new field to mozinfo.json by editing python/mozbuild/mozbuild/mozinfo.py, ran config.status, and the new field doesn't show up. Running configure causes mozinfo.json to update.

This could cause clobbers.

mozinfo.py is already chained up to trigger config.status. However, python/mozbuild/mozbuild/config_status.py is doing something funky. IMO we should have mozinfo writing behind a FileAvoidWrite.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Regenerate mozinfo.json as part of build backend

Previously, mozinfo.json was only generated as configure time.
Unfortunately, the build dependencies did not capture this relationship.
So, changes to mozinfo.py (or any supporting Python file) would not
trigger mozinfo regeneration, possibly leading to clobbers.

This patch moves mozinfo.json generation from the body of config.status
to the build backend. We had to add an AC_SUBST so the build config
knows when to build mozinfo.json. This was needed because js/src's build
system doesn't define all the required variables to create mozinfo.json.
Once js/src's configure/config.status is merged into the main build
config tree, this workaround can be removed.

While we were here, mozinfo.json was made to have consistent output and
its changes are now viewable with config.status --diff.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8371234 [details] [diff] [review]
Regenerate mozinfo.json as part of build backend

Review of attachment 8371234 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +8925,5 @@
>  
> +dnl Conditional writing of mozinfo can likely go away once js/src's configure
> +dnl is merged into us.
> +WRITE_MOZINFO=1
> +AC_SUBST(WRITE_MOZINFO)

So, now that js/src/config.status does nothing except on js standalone builds, you just need to make that happen when JS_STANDALONE is not set.

::: python/mozbuild/mozbuild/backend/common.py
@@ +235,5 @@
> +        # automation.
> +        path = mozpath.join(self.environment.topobjdir, 'mozinfo.json')
> +        if self.environment.substs.get('WRITE_MOZINFO'):
> +            with self._write_file(path) as fh:
> +                write_mozinfo(fh, self.environment, os.environ)

I'm not sure what moving this to the backend buys us.

Comment 3

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #2)
> ::: python/mozbuild/mozbuild/backend/common.py
> @@ +235,5 @@
> > +        # automation.
> > +        path = mozpath.join(self.environment.topobjdir, 'mozinfo.json')
> > +        if self.environment.substs.get('WRITE_MOZINFO'):
> > +            with self._write_file(path) as fh:
> > +                write_mozinfo(fh, self.environment, os.environ)
> 
> I'm not sure what moving this to the backend buys us.

I could leave it in the main body of config.status, sure. But mozinfo.json is the file being written directly from config.status and not the backend. It's a one-off. Let's make it conform.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Regenerate mozinfo.json as part of build backend

Rebased on top of js/src refactoring.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8375934 [details] [diff] [review]
Regenerate mozinfo.json as part of build backend

Review of attachment 8375934 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +8758,2 @@
>  MOZ_CREATE_CONFIG_STATUS()
> +unset WRITE_MOZINFO

You should just remove WRITE_MOZINFO and use !BUILDING_JS.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Regenerate mozinfo.json as part of build backend

The issue with the js/src/mozinfo.json file was that mach and the
environment detection was picking it up and pinning js/src to topobjdir.

Now that js/src's config.status is merged into the top-level, we don't
have 2 mozinfo.json files. We'll still create a mozinfo.json for JS
standalone builds. But I believe this should be harmless.

https://tbpl.mozilla.org/?tree=Try&rev=fb40911e6ee2

Comment 7

[Gregory Szorc [:gps]]

Attachment: Regenerate mozinfo.json as part of build backend

I re-added WRITE_MOZINFO. I didn't make the backend key off
JS_STANDALONE because I would have had to update a bunch of tests.
This approach is the simplest and least amount of work.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #7)
> Created attachment 8377915 [details] [diff] [review]
> Regenerate mozinfo.json as part of build backend
> 
> I re-added WRITE_MOZINFO. I didn't make the backend key off
> JS_STANDALONE because I would have had to update a bunch of tests.
> This approach is the simplest and least amount of work.

I don't see why. If I take the previous patch and add this:

diff --git a/python/mozbuild/mozbuild/backend/common.py b/python/mozbuild/mozbuild/backend/common.py
--- a/python/mozbuild/mozbuild/backend/common.py
+++ b/python/mozbuild/mozbuild/backend/common.py
@@ -237,17 +237,17 @@ class CommonBackend(BuildBackend):
 
         self._handle_webidl_collection(self._webidls)
 
         for config in self._configs:
             self.backend_input_files.add(config.source)
 
         # Write out a JSON file used by test harnesses, other parts of
         # automation.
-        if self._write_mozinfo:
+        if self._write_mozinfo and not 'JS_STANDALONE' in self.environment.substs:
             path = mozpath.join(self.environment.topobjdir, 'mozinfo.json')
             with self._write_file(path) as fh:
                 write_mozinfo(fh, self.environment, os.environ)
 
         # Write out a machine-readable file describing every test.
         path = mozpath.join(self.environment.topobjdir, 'all-tests.json')
         with self._write_file(path) as fh:
             json.dump(self._test_manager.tests_by_path, fh, sort_keys=True,

All tests pass.

Comment 9

[Gregory Szorc [:gps]]

Automation failed.

https://tbpl.mozilla.org/php/getParsedLog.php?id=34859673&tree=Try&full=1

Perhaps JS_STANDALONE isn't defined for JS hazards builds?

Comment 10

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #9)
> Automation failed.
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=34859673&tree=Try&full=1
> 
> Perhaps JS_STANDALONE isn't defined for JS hazards builds?

There is no test to exclude JS_STANDALONE in that try push.

Comment 11

[Gregory Szorc [:gps]]

Attachment: Regenerate mozinfo.json as part of build backend

Checking for JS_STANDALONE in the backend.

Comment 12

[Gregory Szorc [:gps]]

https://tbpl.mozilla.org/?tree=Try&rev=90874c41222d

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8380989 [details] [diff] [review]
Regenerate mozinfo.json as part of build backend

Review of attachment 8380989 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/common.py
@@ +171,5 @@
>          self._test_manager = TestManager(self.environment)
>          self._webidls = WebIDLCollection()
>          self._configs = set()
>  
> +        self._write_mozinfo = True # For testing

Could be an argument when instantiating.

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2912b402523d

Comment 15

[Ed Morley [:emorley]]

Backed out:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0a53f6e5853d

For:
16:57:11 - edmorley|sheriffduty: gps: there's orange on your inbound push - might it perhaps need a one-off clobber?
16:57:31 - edmorley|sheriffduty: gps: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=2912b402523d

(Couldn't wait any longer for a reply, sorry)

If this does need a one-off clobber, please can it land with the CLOBBER file updated.

Comment 16

[Gregory Szorc [:gps]]

This shouldn't have required a clobber.

I'm scratching my head over this one.