Closed Bug 1449623 Opened 2 years ago Closed 2 years ago

Export MOZ_BUILD_DATE in the environment to variables.py in the tup backend

Categories

(Firefox Build System :: General, enhancement)

3 Branch
enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mshal, Assigned: mshal)

References

Details

Attachments

(2 files)

The tup backend can rebuild targets when an environment variable has changed, but it needs to explicitly list them in the Tupfile for them to be used by a subprocess. However, the actual environment variables that are needed are not listed anywhere (eg: in a moz.build file). For example, build/variables.py can use MOZ_BUILD_DATE from the environment to generate the buildid. The tup backend currently fails in automation because MOZ_BUILD_DATE is not currently exported in the Tupfile for running variables.py.

I'd like to provide a way to list the environment variables needed by GENERATED_FILES so that when they change, tup can do a minimal rebuild.

Another option is to always export variables like MOZ_BUILD_DATE, but that will mean we do a full rebuild when they change. We currently do this for SHELL/COMSPEC/MOZILLABUILD, since they are needed by mach/mixin/process.py do determine the shell environment. Those variables are also unlikely to change, so it is not worth listing them in moz.build because the impact on an incremental build is minimal.
(In reply to Michael Shal [:mshal] from comment #0)
> The tup backend can rebuild targets when an environment variable has
> changed, but it needs to explicitly list them in the Tupfile for them to be
> used by a subprocess. However, the actual environment variables that are
> needed are not listed anywhere (eg: in a moz.build file). For example,
> build/variables.py can use MOZ_BUILD_DATE from the environment to generate
> the buildid. The tup backend currently fails in automation because
> MOZ_BUILD_DATE is not currently exported in the Tupfile for running
> variables.py.
> 
> I'd like to provide a way to list the environment variables needed by
> GENERATED_FILES so that when they change, tup can do a minimal rebuild.
> 
> Another option is to always export variables like MOZ_BUILD_DATE, but that
> will mean we do a full rebuild when they change. We currently do this for
> SHELL/COMSPEC/MOZILLABUILD, since they are needed by mach/mixin/process.py
> do determine the shell environment. Those variables are also unlikely to
> change, so it is not worth listing them in moz.build because the impact on
> an incremental build is minimal.

This seems like a poor approach, since environment variables are, rather by definition, transient; and we don't want transient things impacting the build.

Shouldn't we prefer to reify the environment into the build configuration (right now, that happens via mozconfig files) and to explicitly incorporate that data into GENERATED_FILES invocations (right now, that happens via flags)?

The proposed approach seems to lead us to a very slippery slope, where scripts depend on os.environ and not on buildconfig.{substs,defines} or flags.
Flags: needinfo?(mshal)
(In reply to Nick Alexander :nalexander from comment #3)
> Shouldn't we prefer to reify the environment into the build configuration
> (right now, that happens via mozconfig files) and to explicitly incorporate
> that data into GENERATED_FILES invocations (right now, that happens via
> flags)?

So the only place we'd really check the environment is during configure then, right? That sounds like a reasonable idea to me. Looking at this further, I'm not sure how many more instances we'd have of using .env aside from MOZ_BUILD_DATE, though it's a little hard to look at just those python files invoked during 'mach build'. So this is probably too much to add if it's just that one instance.

> The proposed approach seems to lead us to a very slippery slope, where
> scripts depend on os.environ and not on buildconfig.{substs,defines} or
> flags.

The goal here was to provide the information needed to the backend to build what we have in the tree today, not to provide a direction for the future. IOW: we currently do use MOZ_BUILD_DATE as an environment variable in build/variables.py rather than as a subst, and the tup backend needs to know about that somehow. I can work around that in the tup backend for now so that I can get it building in automation, and we can look at removing os.environ usage in a followup if that's desirable.
Flags: needinfo?(mshal)
Attachment #8963247 - Flags: review?(core-build-config-reviews)
Attachment #8963248 - Flags: review?(core-build-config-reviews)
Summary: Tag GENERATED_FILES with environment variables → Export MOZ_BUILD_DATE in the environment to variables.py in the tup backend
(In reply to Michael Shal [:mshal] from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> > Shouldn't we prefer to reify the environment into the build configuration
> > (right now, that happens via mozconfig files) and to explicitly incorporate
> > that data into GENERATED_FILES invocations (right now, that happens via
> > flags)?
> 
> So the only place we'd really check the environment is during configure
> then, right? That sounds like a reasonable idea to me.

This is relevant to the l10n parts of the build system, which use/abuse environment variables (and Make variable passing) to achieve things that smell similar to me.

 Looking at this
> further, I'm not sure how many more instances we'd have of using .env aside
> from MOZ_BUILD_DATE, though it's a little hard to look at just those python
> files invoked during 'mach build'. So this is probably too much to add if
> it's just that one instance.

I feel like the way to learn this would be to instrument the existing Python module tracking that mach does (to trace dependencies in the build system) and try to replace os.environ with a dict that also tracked queries.  That might be very close to correct, and easy; or it might be a rat's nest of edge cases :/

> > The proposed approach seems to lead us to a very slippery slope, where
> > scripts depend on os.environ and not on buildconfig.{substs,defines} or
> > flags.
> 
> The goal here was to provide the information needed to the backend to build
> what we have in the tree today, not to provide a direction for the future.
> IOW: we currently do use MOZ_BUILD_DATE as an environment variable in
> build/variables.py rather than as a subst, and the tup backend needs to know
> about that somehow. I can work around that in the tup backend for now so
> that I can get it building in automation, and we can look at removing
> os.environ usage in a followup if that's desirable.

Hmm.  In principle I'm happy to accept an imperfect approach in order to unblock the Tup work, which I believe to be valuable.  Perhaps gps would like to comment on where he sees the balance in this case?  (For the record, I have not looked at the patch -- I'm only commenting on the conceptual approach.)
(In reply to Michael Shal [:mshal] from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> > Shouldn't we prefer to reify the environment into the build configuration
> > (right now, that happens via mozconfig files) and to explicitly incorporate
> > that data into GENERATED_FILES invocations (right now, that happens via
> > flags)?
> 
> So the only place we'd really check the environment is during configure
> then, right? That sounds like a reasonable idea to me. Looking at this
> further, I'm not sure how many more instances we'd have of using .env aside
> from MOZ_BUILD_DATE, though it's a little hard to look at just those python
> files invoked during 'mach build'. So this is probably too much to add if
> it's just that one instance.
> 
> > The proposed approach seems to lead us to a very slippery slope, where
> > scripts depend on os.environ and not on buildconfig.{substs,defines} or
> > flags.
> 
> The goal here was to provide the information needed to the backend to build
> what we have in the tree today, not to provide a direction for the future.
> IOW: we currently do use MOZ_BUILD_DATE as an environment variable in
> build/variables.py rather than as a subst, and the tup backend needs to know
> about that somehow. I can work around that in the tup backend for now so
> that I can get it building in automation, and we can look at removing
> os.environ usage in a followup if that's desirable.

We may be doing this now to accommodate cases the environment changes more often than configure runs, which could be relevant for an incremental build in automation that cares about MOZ_BUILD_DATE.
Attachment #8963248 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Attachment #8963247 - Flags: review?(core-build-config-reviews) → review?(cmanchester)
Comment on attachment 8963248 [details]
Bug 1449623 - Rework exports in the tup backend;

https://reviewboard.mozilla.org/r/232128/#review237720
Attachment #8963248 - Flags: review?(cmanchester) → review+
Comment on attachment 8963247 [details]
Bug 1449623 - Export MOZ_BUILD_DATE for buildid.h;

https://reviewboard.mozilla.org/r/232126/#review237724
Attachment #8963247 - Flags: review?(cmanchester) → review+
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a0ae010bb5a
Rework exports in the tup backend; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/9d74dd42478a
Export MOZ_BUILD_DATE for buildid.h; r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/9a0ae010bb5a
https://hg.mozilla.org/mozilla-central/rev/9d74dd42478a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.