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

Tests wheel reproducibility against version control #243

Merged
merged 4 commits into from
Apr 20, 2021

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented Apr 7, 2021

We were already checking that building a wheel twice yields the same
checksum both times, but we weren't confirming that the checksum matched
what's already committed to version control. Let's do that.

Still leaving the 'reprotest' check enabled because it supports multiple
methods for checking reproducible, which should help us shake out
problems like #231.

Closes #241.

Testing

  1. CI must be passing
  2. Run pytest -k test_wheel_builds_match_version_control tests locally and confirm passing
  3. Run echo this-is-not-a-wheel > localwheels/MarkupSafe-1.1.1-cp37-cp37m-linux_x86_64.whl && gc -m 'TEMPORARY: broken wheel' localwheels/, then re-run step 2, and confirm the test fails.

Step 3 ensures that if a wheel build emits a different checksum from what's already version controlled, CI will fail on it.

@conorsch
Copy link
Contributor Author

conorsch commented Apr 7, 2021

CI is not passing: https://circleci.com/gh/freedomofpress/securedrop-debian-packaging/8074 likely due to #231. Marking as draft until resolved.

@conorsch conorsch marked this pull request as draft April 7, 2021 23:55
@sssoleileraaa
Copy link
Contributor

yeah, #231 is acting sus, however, i can't repro. I get the same reproducible wheel on different machines with different build roots :/

@conorsch
Copy link
Contributor Author

conorsch commented Apr 8, 2021

Shelving this work for now, to focus on #231. Have a draft PR open for comment. Assuming we can resolve #231, we should be unblocked for further work here. To be clear, I haven't actually verified that #231 (i.e. build_path variation) is what's breaking CI here, it's just a hunch.

We were already checking that building a wheel twice yields the same
checksum both times, but we weren't confirming that the checksum matched
what's already committed to version control. Let's do that.

Still leaving the 'reprotest' check enabled because it supports multiple
methods for checking reproducible, which should help us shake out
problems like #231.
@conorsch conorsch force-pushed the 241-wheels-repro-against-vc branch from 1842e85 to 9a0cee5 Compare April 14, 2021 01:02
@conorsch
Copy link
Contributor Author

Will take a closer look at CI now that #246 is merged. At a glance, the CI run times are a good 10m longer or so now. Perhaps we should skip the old reprotest test for wheels and only use this new test, which effectively verifies reproducibility by comparing to vc.

Now we are using the Debian provided Python to create the
virtualenv. Means the wheels will also be created using the system
python, and will have the exact same sha256sums.

By default the CircleCI images are based on the docker Python images
and they install python from building the source.
@kushaldas
Copy link
Contributor

While trying to identify the differences between the wheels generated via the CI and our wheels,
I noticed the following diff.

│ │   The Directory Table (offset 0x1b):
│ │    1        src/markupsafe
│ │ -  2        /usr/include/x86_64-linux-gnu/bits
│ │ -  3        /usr/lib/gcc/x86_64-linux-gnu/8/include
│ │ +  2        /usr/lib/gcc/x86_64-linux-gnu/8/include
│ │ +  3        /usr/include/x86_64-linux-gnu/bits
│ │    4        /usr/include/x86_64-linux-gnu/bits/types
│ │    5        /usr/include
│ │ -  6        /usr/include/python3.7m
│ │ +  6        /usr/local/include/python3.7m
│ │    7        /usr/include/x86_64-linux-gnu/sys

If I look at the Dockerfile for circleci/python:3.7-buster, I can see it is based on the official
Python image from Docker

Now that image shows that it installs Python executable build from source via the https://python.org
and uses the same as default Python.

But, all of our wheels are bulit via the system python provided by Debian Buster.

I now added another commit, where we are using the Debian provided Python.

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

At a glance, the CI run times are a good 10m longer or so now. Perhaps we should skip the old reprotest test for wheels and only use this new test, which effectively verifies reproducibility by comparing to vc.

Yes, I think removing the old reprowheels test is okay. We are verifying against the lfs checked in version here in this new code.

@conorsch after you mark this PR as ready for review, we can do the formal review and merge this.

repo_url = f"https://github.com/freedomofpress/{repo_name}"
build_cmd = f"./scripts/build-sync-wheels -p {repo_url} --clobber".split()
subprocess.check_call(build_cmd)
check_cmd = "git diff --exit-code".split()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is simple.

@emkll emkll marked this pull request as ready for review April 15, 2021 22:34
Conor Schaefer added 2 commits April 16, 2021 09:40
The version-control comparison effectively verifies that our wheels are
byte-for-byte reproducible, and also ensures that we're tracking 100% of
dependencies in version-control already, since the
@conorsch
Copy link
Contributor Author

Ready for final review. See the most recent commits, @kushaldas, which disable the reprotest invocations but preserve the new logic of comparing the built wheels to version control. It's a bit aggressive to fail the tests if the git repo is dirty, since that'll often be the case for developers, but in the CI context that's precisely what we want: any deviation from our vc should raise an error.

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

All good. Approved.

In future we may want to have a night repro test for the wheels.

@kushaldas kushaldas merged commit 7768326 into main Apr 20, 2021
subprocess.check_call("git diff --exit-code".split())
# Check for new, untracked files. Test will fail if working copy is dirty
# in any way, so mostly useful in CI.
assert subprocess.check_output("git status --porcelain".split()) == b""
Copy link
Contributor

Choose a reason for hiding this comment

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

I should keep this command in mind. Thank you @conorsch.

@kushaldas kushaldas deleted the 241-wheels-repro-against-vc branch April 20, 2021 08:26
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.

CI should verify a new wheel in localwheels by rebuilding
3 participants