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

Make the DOM codes reproducible #12679

Closed
wants to merge 1 commit into from
Closed

Make the DOM codes reproducible #12679

wants to merge 1 commit into from

Conversation

lamby
Copy link

@lamby lamby commented Nov 2, 2020

Whilst working on the Reproducible Builds effort I noticed that emscripten could not be built reproducibly. This is because it generates a switch/case statement using Python's random.randint/random.uniform functionality.

This patch seeds this random component using the SOURCE_DATE_EPOCH environment variable so that, iff this variable is present, it will deterministically generate the same output.

(I originally filed this bug in Debian as #973595.)

@welcome
Copy link

welcome bot commented Nov 2, 2020

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

Whilst working on the Reproducible Builds effort [0] I noticed that emscripten
could not be built reproducibly.

This is because it generates a switch/case statement using Python's
random.randint/random.uniform functionality.

This patch seeds this random component using the SOURCE_DATE_EPOCH environment
variable [1] so that, iff this variable is present, it will deterministically
generate the same output.

I originally filed this bug in Debian as #973595 [2].

 [0] https://reproducible-builds.org/
 [1] https://reproducible-builds.org/specs/source-date-epoch/
 [2] https://bugs.debian.org/973595
@sbc100
Copy link
Collaborator

sbc100 commented Nov 2, 2020

But that script is not used during any builds, as far I can tell.

It looks like that scripts only job is to create system/include/emscripten/dom_pk_codes.h and it hasnt been run even once since it was initially created back in d7c0b5a (in 2017).

So for sure any use of random in this script is not effecting any builds.

Given that we seem to bascially never use this script I think we should probably consider removing it anyway.

@lamby
Copy link
Author

lamby commented Nov 2, 2020

So for sure any use of random in this script is not effecting any builds.

Interesting, as that script is definitely being run during the Debian build.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 2, 2020

So for sure any use of random in this script is not effecting any builds.

Interesting, as that script is definitely being run during the Debian build.

Can you point me to where that is?

@sbc100
Copy link
Collaborator

sbc100 commented Nov 2, 2020

I do agree this script should be deterministic though (otherwise all the value will change each time it is run which is clearly not indeneded).. so perhaps a constant seed is better.

@lamby
Copy link
Author

lamby commented Nov 2, 2020

Looks like that's here:

https://sources.debian.org/src/emscripten/2.0.8~dfsg-6/debian/rules/#L171

(As for exactly why this was added, we would need to loop in the Debian maintainer for this particular package; we have reached the extent of my insight, alas.)

@sbc100
Copy link
Collaborator

sbc100 commented Nov 2, 2020

Oh man.. that line should certainly be removed.

I'm working on some other fixes to that file now.

In the mean time, if you would like to land this change can you use a fixed seed?

@lamby
Copy link
Author

lamby commented Nov 2, 2020

Sorry for any confusion but I'm not the Debian maintainer for this package so I am ultimately not in the position of being able to comment on whether that line can/should be removed - I merely found the unreproducibility and let you kind folks know. :) I would guess a fixed, hardcoded seed would work; indeed, this is in essence what my patch does in its own way. :)

sbc100 added a commit that referenced this pull request Nov 2, 2020
Embed the current hash seed values in the script so
that running it repeatendly doesn't change the output.

See #12679
sbc100 added a commit that referenced this pull request Nov 2, 2020
Embed the current hash seed values in the script so
that running it repeatendly doesn't change the output.

See #12679
sbc100 added a commit that referenced this pull request Nov 2, 2020
Embed the current hash seed values in the script so
that running it repeatedly doesn't change the output.

See #12679
@sbc100
Copy link
Collaborator

sbc100 commented Nov 3, 2020

I've landed two changes that should make this no longer a problem.

If you feel like following up with downstream, that line should still be removed from the debian/rules file.

@sbc100 sbc100 closed this Mar 8, 2021
@sbc100 sbc100 deleted the branch emscripten-core:master March 8, 2021 23:50
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

2 participants