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

Rebuild all wheels reproducibly and use wheels from local filesystem #213

Merged
merged 5 commits into from
Jan 26, 2021

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Jan 5, 2021

This patch stops accessing the webbased index for the wheels,
instead uses the local filesystem path.

Fixes #212 (work remaining)

Next step in this PR.

  • Someone with signing capability should regenerate all the wheels for securedrop-client, securedrop-proxy and securedrop-log projects. ./scripts/build-sync-wheels --clobber -p ../securedrop-client is one example.
  • Then we should regenerate the build-requirements.txt files for all the 3 Debian packages
  • CI should be green
  • Remove our python package index.(temporarily via infra work)
  • CI still should be green.

How to test?

  • make securedrop-client
  • make securedrop-proxy
  • make securedrop-log
  • CI should be green.

All of the above commands should pass.

This patch stops accessing the webbased index for the wheels,
instead uses the local filesystem path.
@conorsch conorsch self-requested a review January 5, 2021 16:37
Conor Schaefer added 2 commits January 5, 2021 12:13
Adjusting the error-handling based on a recent signing operation.
Makes the tooling a bit friendlier for maintainers.
In order to regenerate the wheel files, I ran:

  ./scripts/build-sync-wheels --clobber -p ../securedrop-client
  ./scripts/build-sync-wheels --clobber -p ../securedrop-log
  ./scripts/build-sync-wheels --clobber -p ../securedrop-proxy

Each repo had the latest "main" branch commit checked out. Then I ran
./scripts/sync-sha256sums, completed the signing ceremony, and
committed the results.
@conorsch
Copy link
Contributor

conorsch commented Jan 5, 2021

regenerate all the wheels

@kushaldas I've appended new wheel files and checksums here, and marked the task as complete in your list. Back over to you!

@eloquence eloquence changed the title Uses wheels from local filesystem Rebuild all wheels reproducibly and use wheels from local filesystem Jan 6, 2021
@eloquence
Copy link
Member

eloquence commented Jan 6, 2021

(Retitled for clarity, please don't hesitate to edit if I misunderstood but I wanted to make it clearer that this PR now also updates all the existing wheels.)

kushaldas added a commit to freedomofpress/securedrop-proxy that referenced this pull request Jan 6, 2021
The new wheels are coming in the following PR:

freedomofpress/securedrop-builder#213

These wheels are reproducible, you can build them following our
Makefile.
kushaldas added a commit to freedomofpress/securedrop-client that referenced this pull request Jan 6, 2021
The new wheels are coming in the following PR:

freedomofpress/securedrop-builder#213

These wheels are reproducible, you can build them following our
Makefile.
kushaldas added a commit to freedomofpress/securedrop-log that referenced this pull request Jan 6, 2021
The new wheels are coming in the following PR:

freedomofpress/securedrop-builder#213

These wheels are reproducible, you can build them following our
Makefile.
@kushaldas
Copy link
Contributor Author

Next, we will have to merge these 3 PRs:

Only after those are merged, we will be able to see CI progressing for this PR.

kushaldas added a commit to freedomofpress/securedrop-log that referenced this pull request Jan 7, 2021
The new wheels are coming in the following PR:

freedomofpress/securedrop-builder#213

These wheels are reproducible, you can build them following our
Makefile.
kushaldas added a commit to freedomofpress/securedrop-client that referenced this pull request Jan 7, 2021
The new wheels are coming in the following PR:

freedomofpress/securedrop-builder#213

These wheels are reproducible, you can build them following our
Makefile.
@conorsch
Copy link
Contributor

conorsch commented Jan 8, 2021

@kushaldas I haven't provided in-depth review of all the checksums in the component PRs you link to, but plan to do so tomorrow. While testing that, I might append yet another test-only commit onto this PR, tracking the feature branches in those repos, to confirm everything is green prior to approving.

Thanks for preparing! Looking forward to landing these changes.

@kushaldas kushaldas force-pushed the use_reproducible_wheels branch from b9334ea to 71de804 Compare January 11, 2021 09:43
@kushaldas
Copy link
Contributor Author

So the package build tests were failing as they were building the last release. And the repository now has different wheels from the last release. In my temporary commit, I am doing night build instead of rebuilding the last release.

I am still not sure how to fix the reproducibility-checks test for this purpose. @conorsch please let me know what do you think.

@conorsch
Copy link
Contributor

And the repository now has different wheels from the last release.

From the last release of which package? Are you recommending that the wheels in this PR be regenerated to resolve?

@kushaldas
Copy link
Contributor Author

So the package build tests were failing as they were building the last release. And the repository now has different wheels from the last release. In my temporary commit, I am doing night build instead of rebuilding the last release.

I am still not sure how to fix the reproducibility-checks test for this purpose. @conorsch please let me know what do you think.

And the repository now has different wheels from the last release.

From the last release of which package? Are you recommending that the wheels in this PR be regenerated to resolve?

  • securedrop-client
  • securedrop-proxy
  • securedrop-log

Generally the CI is trying to build the last release of these packages, which is failing because now we have new wheels. My temporary commit tries to build the nightly, that is why is passing for those builds. But, the reproducibility-checks test tries to build from the old tarballs, which contains the old wheel checkcums. This is why https://circleci.com/gh/freedomofpress/securedrop-debian-packaging/6586?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link is still failing.

@emkll
Copy link
Contributor

emkll commented Jan 11, 2021

I am still not sure how to fix the reproducibility-checks test for this purpose.

@kushaldas you correctly identify that the reproducibility check rely on the archived tarballs[1], which were have considered deprecating. It sounds like we could replace this step by manually cloning and building the tarball locally on the latest release tag on main branch of the target repos using some of the logic introduced in [2]. You can then, for the purposes of this PR, add a temporary commit to check out the feature branch with the updated hashes.
[1] https://github.com/freedomofpress/securedrop-debian-packaging/blob/main/.circleci/config.yml#L484
[2] #185

684a38a6f903c1d71d6d5fac066b58d7768af4de2b832e426ec79c30daa94a16 idna-2.7.tar.gz
614c22fe1a5b0a3f46f6c5c43ff2e6795e4e784328d559ec9dc49db0f06b3a75 Mako-1.0.7-py3-none-any.whl
99d041a616a3655725dffe459916627b75640a7d045ed33f51ce158a168ca3ef Mako-1.0.7-py3-none-any.whl
4e02fde57bd4abb5ec400181e4c314f56ac3e49ba4fb8b0d50bba18cb27d25ae Mako-1.0.7.tar.gz
c6b726d2e9d6300a044cf6a37627f10994268d6ac39464bc0d725126609311a5 MarkupSafe-1.0-py3-none-any.whl
Copy link
Contributor

@sssoleileraaa sssoleileraaa Jan 12, 2021

Choose a reason for hiding this comment

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

can 1.0 entries be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can 1.0 entries be removed?

I think we should do it later with a new PR as we mess up, it will difficult to figure out where it broke (if it is the new work or removing old things).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I guess you are talking about removing the old MarkupSafe wheel removal, that can be done.

@kushaldas kushaldas force-pushed the use_reproducible_wheels branch from 291363f to 14b2f75 Compare January 12, 2021 10:44
@kushaldas
Copy link
Contributor Author

I could not do it via packages/tarballs from current main of those packages, as I will have to remove the signing check and all related checks etc. Can anyone please try to write it in a better way?

@conorsch
Copy link
Contributor

I haven't taken a look at this yet, @kushaldas, but will do so later in the week.

@kushaldas
Copy link
Contributor Author

@conorsch I tried it in #214 and I think finally I got it properly.
There I am rebuilding the tarball for securedrop-proxy and securedrop-log and disable gpg signature. We can enable gpg signature after the next release of those packages.

@conorsch conorsch mentioned this pull request Jan 16, 2021
@conorsch
Copy link
Contributor

@kushaldas Nice work in #214! I took the liberty of appending yet more partial work, specifically using reprotest to verify that the checksums matched on the .deb files, same as on the .whl files (#211). As part of that work, I pulled in your test-only commits into this PR, and I've also re-broken the tests. As part of final review, let's make sure to drop all the test-only commits.

Based on the new changes, I have a couple immediate observations:

  1. The securedrop-export package needs its own PR, since it's still building from the pypi.securedrop.org/ index on this branch
  2. reprotest requires that build artifacts be relative to the project directory. That's already true of the whls, but the debs we've been placing in ~/debbuild/packaging/, and I propose we move that logic to ./build/debbuild/packaging/ in this PR. I've tried to make that migration here, but it's not passing in CI yet.
  3. Assuming make test passes for you locally, and in CI, it looks like we can remove the following CI checks:
  • build-buster-securedrop-client
  • build-buster-securedrop-proxy
  • build-buster-securedrop-log
  • build-buster-securedrop-export (after we have the checksums updated, mentioned above)

I'm suggesting removal of those tests on every PR run, because the reprotest suite effectively verifies that the build passes—and then passes again! If you agree, please take a stab at adjusting the settings. I'd also appreciate your help on getting the new reprotest suite passing in CI. You'll see that I've moved it from a machine to container executor in CI, but that itself isn't enough. See what you can figure out. We're almost there! 🚀

@conorsch
Copy link
Contributor

Took another look at this today. Looks like we're running into a CI-specific problem with setarch, just as we saw in e293606. It appears this problem prevents us from using reprotest to verify the deb package builds, because we can either use Focal VM images, which cause the wheel lookups to fail, since Focal uses python3.8 rather than Buster's 3.7, or we can use a Buster container image, which will fail on setarch. I haven't found a way to tell reprotest to skip the setarch line, so I believe we're blocked on that for now.

I'll work on adjusting the tests to accommodate. Overall, the logic here is very much what we want! Will poke at reprotest in CircleCI containers another day to understand the root cause better.

@emkll
Copy link
Contributor

emkll commented Jan 21, 2021

@conorsch @kushaldas have you tried the "next gen" circle images? They have python buster images that can be used here:
https://circleci.com/docs/2.0/circleci-images/#python

We have recently switched to these for some core CI targets: freedomofpress/securedrop#5720

@conorsch
Copy link
Contributor

No, but worth a shot! Thanks for mentioning.

@conorsch conorsch self-assigned this Jan 25, 2021
More pytest-based reprotest invocations, this time focusing on .deb
files. Replaces the CircleCI repro tests that manually compared hashes.

Also modifies build script to support commit hash

The logic assumed we were always building from a prod release tag.
As a result, the CI logic was reimplementing the tarball mangling.
Let's make the script more flexible, so we can run the script in CI and
thereby get a bit more test coverage for it.

Modifies CI env for reprotest support

When building .deb packages, we need the python version for the
packaging environment to match that of the target platform, i.e.
python3.7 for buster. In CI, our platform options are:

  * VM, Ubuntu 20.04
  * Container, Debian 10

The container driver in CircleCI does not permit "setarch" calls,
erroring out immediately. The setarch calls are not optional in
reprotest, unfortunately, so let's hack the file and remove it entirely,
only in CI.
@conorsch conorsch force-pushed the use_reproducible_wheels branch from 43d6a6d to 96d9e59 Compare January 25, 2021 23:44
Updated the build logic for securedrop-export to remove use of the
https://pypi.securedrop.org URL. At present, the securedrop-export
doesn't use any external Python dependencies, it's core-only. So the
custom PyPI URL was never necessary, and technically we could excise it
altogether right now, but for consistency's sake I'm updating it to
match the other Python-based SDW packages.
@conorsch conorsch force-pushed the use_reproducible_wheels branch from 96d9e59 to ebc1a78 Compare January 26, 2021 00:11
@conorsch
Copy link
Contributor

The securedrop-export package needs its own PR, since it's still building from the pypi.securedrop.org/ index on this branch

Fixed in this branch. A clarification, though: yes, the build logic was erroneously using pypi.securedrop.org, but the securedrop-export package doesn't have any external dependencies, so that wasn't breaking the build. Therefore no additional PR is required. Updated the docs to remove mention of the pypi.securedrop.org URL entirely, since we plan to remove it in the future.

have you tried the "next gen" circle images?

I tried those, but to no avail. The problem appears to be that in CircleCI, containers are run with reduced privileges, prohibiting the use of the setarch call. Unfortunately reprotest doesn't allow us to disable that call via configuration, so I patched the file only in the CI env: https://github.com/freedomofpress/securedrop-debian-packaging/pull/213/files#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47R306 Not the prettiest solution, but it lets us remove the manual checksum comparison specific to the CircleCI testing logic. The reproducibility checks can now be run locally, in addition to CI, which should help greatly in future debugging sessions.

CI is now passing across the board, so I'm marking ready and proceeding with final review.

@conorsch conorsch marked this pull request as ready for review January 26, 2021 00:21
conorsch pushed a commit to freedomofpress/securedrop-log that referenced this pull request Jan 26, 2021
The new wheels are coming in the following PR:

freedomofpress/securedrop-builder#213

These wheels are reproducible, you can build them following our
Makefile.
conorsch pushed a commit to freedomofpress/securedrop-client that referenced this pull request Jan 26, 2021
The new wheels are coming in the following PR:

freedomofpress/securedrop-builder#213

These wheels are reproducible, you can build them following our
Makefile.
@conorsch
Copy link
Contributor

Merged the associated component PRs, then reverted the temporary commits on this branch. See fully passing build here, with temporary commits reverted: https://app.circleci.com/pipelines/github/freedomofpress/securedrop-debian-packaging/885/workflows/3fb0f623-77a7-4b16-9c9b-2497e8866f5c I'll rebase again to drop those temporary commits entirely, confirm passing, then merge.

@conorsch conorsch force-pushed the use_reproducible_wheels branch from 9d6c143 to da78f9b Compare January 26, 2021 00:55
@conorsch conorsch merged commit d05ecd3 into main Jan 26, 2021
@conorsch
Copy link
Contributor

After merging all associated PRs, including this one, into the main branches for all repos, I re-ran the tests in CircleCI, and they're showing green across the board:

There may be a few PRs, especially in the securedrop-client repo, that will need to be rebased to keep tests passing. Other than that, no further action required.

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.

Rebuild all the current wheels in a reproducible way & use them for package building
5 participants