Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate shell environment variable SOURCE_DATE_EPOCH #4239

Closed
wants to merge 1 commit into from

Conversation

bmwiedemann
Copy link
Contributor

@bmwiedemann bmwiedemann commented Sep 25, 2022

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

This is a PoC patch to
Preserve SOURCE_DATE_EPOCH for reproducible builds.

https://reproducible-builds.org/specs/source-date-epoch/ says

Build processes MUST NOT unset this variable for child processes if it is already present.

Without this patch, openSUSE's games/globulation2 builds varied from a .cpp file with __TIME__ that should be normalized by g++ as we set SOURCE_DATE_EPOCH.

Signed-off-by: Bernhard M. Wiedemann

I am not familiar with the scons codebase, thus would appreciate help to get this improved.

for reproducible builds.

https://reproducible-builds.org/specs/source-date-epoch/
says
>  Build processes MUST NOT unset this variable for child processes if it is already present.

Without this patch, openSUSE's games/globulation2 builds varied from
a .cpp file with __TIME__ that should be normalized by g++ as we
set SOURCE_DATE_EPOCH.

Signed-off-by: Bernhard M. Wiedemann <bwiedemann@suse.de>
@mwichmann
Copy link
Collaborator

scons uses this for its own build (see site_scons/BuildCommandLine.py )

@bdbaddog
Copy link
Contributor

This would violate one of SCons's primary promises that the external environment doesn't impact the build.
That said I can see a reason it should be added to the (currently only 4) exceptions.

The way you've currently written it would ignore any instructions in the build system itself to set env['ENV']['SOURCE_DATE_EPOCH']. So no developer could ever overwrite that. Which I don't think is good.
And that really violates and even more important promise that the build system would control the build environment.

IMHO this should be fixed in games/globulation2's codebase (as @mwichmann pointed out we have such a fix, and in fact you provided that change to SCons' own build system).

Thoughts?

@bmwiedemann
Copy link
Contributor Author

I think, the best would be to add it to exceptions.
I don't want to patch up an unlimited number of projects to have it pass-through SOURCE_DATE_EPOCH... unless globulation2 did something very non-standard that caused the trouble.

@bdbaddog bdbaddog changed the title Preserve SOURCE_DATE_EPOCH Propagate shell environment variable SOURCE_DATE_EPOCH Sep 26, 2022
@bdbaddog
Copy link
Contributor

I think, the best would be to add it to exceptions. I don't want to patch up an unlimited number of projects to have it pass-through SOURCE_DATE_EPOCH... unless globulation2 did something very non-standard that caused the trouble.

That's a concern for your build environment.

We have to consider the effects of breaking SCons policy on propagating any shell environment variables without the build system explicitly requesting it.

@@ -1034,6 +1034,8 @@ def __init__(
# should override any values set by the tools.
for key, val in save.items():
self._dict[key] = val
if 'SOURCE_DATE_EPOCH' in os.environ:
self._dict['ENV']['SOURCE_DATE_EPOCH'] = os.environ['SOURCE_DATE_EPOCH']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that this is going to do what you expect. This will set a value in the construction environment, yes, but it still won't be passed on to gcc or whoever needs to be listening during builds. For that, it needs to be added to the Execution Environment (usually env['ENV']).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what worked in my tests (after 8 failed tries). I think, since this is Environment.py the self._dict['ENV'] here is env['ENV']

@dmoody256
Copy link
Contributor

This should be a SCons built in command line option (at the very least) to turn on taking in the value from the environment.

@bmwiedemann
Copy link
Contributor Author

This should be a SCons built in command line option (at the very least) to turn on taking in the value from the environment.

I tend to disagree there.
What is the goal of that no-pass-env policy?
If the goal of the policy to not pass environment variables is to ensure reproducibility of results, then SOURCE_DATE_EPOCH should be added to the exception list, because the outer build environment (e.g. open-build-service / rpmbuild in the case of openSUSE) ensures it is set consistently.

@bdbaddog
Copy link
Contributor

bdbaddog commented Sep 27, 2022

Here's all you need to make this work without changing SCons at all.

Create a directory:
mkdir -p ~/.scons/site_scons

The save the following as ~/.scons/site_scons/site_init.py

import os
import SCons.Environment

old_init = SCons.Environment.Base.__init__

def new_init(self, **kw):
    print("In my custom init")
    old_init(self, **kw)
    if 'SOURCE_DATE_EPOCH' in os.environ:
        self._dict['ENV']['SOURCE_DATE_EPOCH'] = os.environ['SOURCE_DATE_EPOCH']

SCons.Environment.Base.__init__ = new_init

Then as long as you have SOURCE_DATE_EPOCH defined in your shell, any build you build with SCons will propagate this.

@bmwiedemann
Copy link
Contributor Author

~/.scons/site_scons/site_init.py

Could we also add that into a system python path under /usr/lib/... ? That would make it easier to pull into our build env.

@mwichmann
Copy link
Collaborator

There is a predefined "system location", but it's not in the Python path: /usr/share/scons/site_scons.

See https://scons.org/doc/production/HTML/scons-man.html#opt-site-dir

@bdbaddog
Copy link
Contributor

@bmwiedemann - is this workable for your builds?

@bmwiedemann
Copy link
Contributor Author

So putting an extra file into /usr/share/scons/site_scons seems a doable, however it needs to be synchronized to upstream scons updates and Debian+Archlinux will need the same file, so it would be best to add that file into scons git and document its usage by distributions aiming for reproducible builds.

@lamby
Copy link

lamby commented Oct 3, 2022

  • An optional command-line argument would not be practical for distributions (such as Debian) as they would have to first locate and then update every package build script that uses Scons (!).

  • Thanks for providing and suggesting the site_scons mechanism. I think this would provide the best tradeoff between Scons' admirable promise that builds are not affected by the surrounding environment and a way so that distributions to be able to poke through specific items of the outside environment in extremely limited and special circumstances.

  • It would be indeed be preferable if the hook script to "poke through" SOURCE_DATE_EPOCH was shipped with Scons itself. What would be the next steps to get that merged?

@bdbaddog
Copy link
Contributor

bdbaddog commented Oct 3, 2022

  • An optional command-line argument would not be practical for distributions (such as Debian) as they would have to first locate and then update every package build script that uses Scons (!).

The command line argument could work if you set SCONSFLAGS in your shell environment.. No need to update every package which is built.

  • Thanks for providing and suggesting the site_scons mechanism. I think this would provide the best tradeoff between Scons' admirable promise that builds are not affected by the surrounding environment and a way so that distributions to be able to poke through specific items of the outside environment in extremely limited and special circumstances.
  • It would be indeed be preferable if the hook script to "poke through" SOURCE_DATE_EPOCH was shipped with Scons itself. What would be the next steps to get that merged?

What's the best way to indicate to people who would use scons to build packages for distribution that such a file is available and how to use?

Note:Besides you I don't think any other SCons user has ever enquired about SOURCE_DATE_EPOCH thus far..

@bdbaddog
Copy link
Contributor

@bmwiedemann @lamby @mwichmann
I can see the utility of including the script with our package.
How would we best inform distribution packages about this?

@bdbaddog
Copy link
Contributor

Does anyone use the debian directory from our git repo?

@lamby
Copy link

lamby commented Oct 25, 2022

Does anyone use the debian directory from our git repo?

Debian doesn't.

How would we best inform distribution packages about this?

Ideally by pinging the key distributions or by filing a wishlist bug against them (they only need to adjust build scripts to install it once, after all). Hopefully they would read the changelogs extremely carefully, but likely not. :)

bdbaddog added a commit to bdbaddog/scons that referenced this pull request Nov 13, 2022
…ell environment to support reproducible builds. This is replacement logic for PR SCons#4239
@bdbaddog
Copy link
Contributor

Closing in favor of #4261

@bdbaddog bdbaddog closed this Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants