-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-110932: Fix regrtest for SOURCE_DATE_EPOCH #111143
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
Conversation
If the SOURCE_DATE_EPOCH environment variable is defined, use its value as the random seed.
SOURCE_DATE_EPOCH intent is to make programs reproducible. Without this change, if SOURCE_DATE_EPOCH is set, regrtest uses a random seed which makes tests less reproducible. With this change, the random seed is constant and so the behavior should be more reproducible.
|
@sobolevn: Would you mind to review this change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me, just one idea.
else random.getrandbits(32) | ||
) | ||
if 'SOURCE_DATE_EPOCH' in os.environ: | ||
if ('SOURCE_DATE_EPOCH' in os.environ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
if ('SOURCE_DATE_EPOCH' in os.environ | |
if ( | |
'SOURCE_DATE_EPOCH' in os.environ |
# SOURCE_DATE_EPOCH should be an integer, but use a string to not | ||
# fail if it's not integer. random.seed() accepts a string. | ||
# https://reproducible-builds.org/docs/source-date-epoch/ | ||
self.random_seed: int | str = os.environ['SOURCE_DATE_EPOCH'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Myabe we should always use str
? There's no real reason to use int
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is subtle. It avoids to import hashlib to convert a string to an integer in Random.seed(). Right now, hashlib is always imported, but I have a local branch to reduce the number of imports at Python startup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm talking about the second code path, where we create a random seed as an integer.
# https://reproducible-builds.org/docs/source-date-epoch/ | ||
self.random_seed: int | str = os.environ['SOURCE_DATE_EPOCH'] | ||
elif ns.random_seed is None: | ||
self.random_seed = random.getrandbits(32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, it would become:
self.random_seed = random.getrandbits(32) | |
self.random_seed = str(random.getrandbits(32)) |
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
If the SOURCE_DATE_EPOCH environment variable is defined, use its value as the random seed. (cherry picked from commit 7237fb5) Co-authored-by: Victor Stinner <[email protected]>
If the SOURCE_DATE_EPOCH environment variable is defined, use its value as the random seed. (cherry picked from commit 7237fb5) Co-authored-by: Victor Stinner <[email protected]>
GH-111153 is a backport of this pull request to the 3.11 branch. |
GH-111154 is a backport of this pull request to the 3.12 branch. |
Merged, thanks to reviewing the change @sobolevn. |
…1153) gh-110932: Fix regrtest for SOURCE_DATE_EPOCH (GH-111143) If the SOURCE_DATE_EPOCH environment variable is defined, use its value as the random seed. (cherry picked from commit 7237fb5) Co-authored-by: Victor Stinner <[email protected]>
…1154) gh-110932: Fix regrtest for SOURCE_DATE_EPOCH (GH-111143) If the SOURCE_DATE_EPOCH environment variable is defined, use its value as the random seed. (cherry picked from commit 7237fb5) Co-authored-by: Victor Stinner <[email protected]>
If the SOURCE_DATE_EPOCH environment variable is defined, use its value as the random seed.
If the SOURCE_DATE_EPOCH environment variable is defined, use its value as the random seed.
If the SOURCE_DATE_EPOCH environment variable is defined, use its value as the random seed.