mozinfo.json should be regenerated if mozinfo.py changes

NEW
Unassigned

Status

5 years ago
3 months ago

People

(Reporter: gps, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 8371234 [details] [diff] [review]
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.
Attachment #8371234 - Flags: review?(mh+mozilla)
(Reporter)

Updated

5 years ago
Assignee: nobody → gps
Status: NEW → ASSIGNED
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.
Attachment #8371234 - Flags: review?(mh+mozilla) → feedback-
(Reporter)

Comment 3

5 years ago
(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.
(Reporter)

Comment 4

5 years ago
Created attachment 8375934 [details] [diff] [review]
Regenerate mozinfo.json as part of build backend

Rebased on top of js/src refactoring.
Attachment #8375934 - Flags: review?(mh+mozilla)
(Reporter)

Updated

5 years ago
Attachment #8371234 - Attachment is obsolete: true
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.
Attachment #8375934 - Flags: review?(mh+mozilla) → feedback+
(Reporter)

Comment 6

5 years ago
Created attachment 8377671 [details] [diff] [review]
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
Attachment #8377671 - Flags: review?(mh+mozilla)
(Reporter)

Updated

5 years ago
Attachment #8375934 - Attachment is obsolete: true
(Reporter)

Comment 7

5 years ago
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.
Attachment #8377915 - Flags: review?(mh+mozilla)
(Reporter)

Updated

5 years ago
Attachment #8377671 - Attachment is obsolete: true
Attachment #8377671 - Flags: review?(mh+mozilla)
(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.
(Reporter)

Comment 9

5 years ago
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?
(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.
(Reporter)

Comment 11

5 years ago
Created attachment 8380989 [details] [diff] [review]
Regenerate mozinfo.json as part of build backend

Checking for JS_STANDALONE in the backend.
Attachment #8380989 - Flags: review?(mh+mozilla)
(Reporter)

Updated

5 years ago
Attachment #8377915 - Attachment is obsolete: true
Attachment #8377915 - Flags: review?(mh+mozilla)
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.
Attachment #8380989 - Flags: review?(mh+mozilla) → review+
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.
(Reporter)

Comment 16

5 years ago
This shouldn't have required a clobber.

I'm scratching my head over this one.
(Reporter)

Updated

4 years ago
Assignee: gps → nobody
Status: ASSIGNED → NEW

Updated

6 months ago
Product: Core → Firefox Build System

Updated

3 months ago
See Also: → bug 1461795
You need to log in before you can comment on or make changes to this bug.