-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add pipeline for building Debian packages in CircleCI #44
Conversation
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.
Great approach here, very much looking forward to having automated checks. Left some in-line comments suggesting yet more scripting inside the repo, specifically with an eye toward debugging, as well as developer reuse in the near-term.
Ping when you want a full review, looks like we'll be able to get this in no problem once we have full coverage.
- &fetchwheels | ||
run: | ||
name: Download wheels and sources | ||
command: make fetch-wheels |
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.
Hurray reusable steps!
echo 'export LATEST_TAG=$(cat ~/packaging/sd_version)' >> $BASH_ENV | ||
# Create tarball | ||
git checkout $LATEST_TAG | ||
python3 setup.py sdist |
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.
Crafty use of a fileglob to cd
into the target dir without specifying the name. Given that the consolidated build logic operates the same way on all packages, does it make sense to condense these steps into a single wrapper, e.g. build-package
? The only arg we need is the package name, e.g. securedrop-client
.
The major advantage I see to such a wrapper is that it'd be easy to run locally in case we need to debug CI. Also, until we have full CD on these packages, providing guardrails wherever possible on the local dev story seems like it'll save time and minimize mistakes.
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.
Even if you agree it's worth wrapping further, let's definitely keep the separate "jobs" for each package: doing so greatly clarifies the CI results list on PRs, so it'll be obvious if a PR breaks on a specific package.
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.
just flagging that there is a makefile target that encapsulates the deb packaging logic in this repo and takes the package name: make securedrop-client
, make securedrop-proxy
which is called in the subsequent run step
the step you're commenting on here is preparing the release tarball and is something that is ran in the repo to be packaged... while i could add a makefile target for this (or subsume this into the existing makefile target, and have it take the directory where python3 setup.py sdist
should be run), it seems like the wins are pretty small. the other steps above are just 1. getting in the right dir on the CI machine and 2. getting the latest tag and making it available across run steps
name: Get latest tag for the project and make a source tarball | ||
command: | | ||
cd ~/packaging/securedrop-* | ||
export LATEST_TAG="$(git describe --tags $(git rev-list --tags --max-count=1))" |
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.
Nice solution on the tag lookup, I'd have gone with git tag | sort -nr | head -n1
, but this looks more durable. 👍
Note: this is temporarily blocked until we release new versions of securedrop-client and securedrop-proxy. We need this because the debian packaging logic now requires pip-tools, which we added to both securedrop-client and securedrop-proxy in freedomofpress/securedrop-proxy#33 and freedomofpress/securedrop-client#372. Once freedomofpress/securedrop-client#397 is resolved we will make new tags in those repos |
once we've verified the sig, we shouldn't fail just because the order of the sorting in sha256sums.txt and the platform we're verifying on are different
rebased on top of latest master for building test packages for freedomofpress/securedrop-workstation#268 (I need commit 5fc689b in this branch) (recall the point of freedomofpress/securedrop-workstation#268 is to get up new tags in securedrop-proxy and securedrop-client to get this PR unblocked) |
This is now unblocked - I've re-ran the two CI jobs on the latest tags |
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.
Looks great, CI is passing, let's get it in.
This gets us towards #8 and will help catch build breakage (e.g. c0b2e6b which fixes broken securedrop-client package builds) that is currently getting merged due to no CI.
Temporarily blocked until we release new versions of
securedrop-client
andsecuredrop-proxy
(see below).Updated description as PR dependency has since been merged. - @eloquence 2019-06-11