-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat(build-deb): Upload source package and run lintian #22
Conversation
781d029
to
4156a27
Compare
ab58f16
to
7b57d13
Compare
Fix warnings related to: - Version number - File too long - No contributor name - Extra whitespace around name
7b57d13
to
9eb66d6
Compare
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.
Thanks for starting this here, I had few other ideas that I was drafting in ubuntu/authd#130, but this seems a good progress.
|
||
# Sanitize the docker name so that it stick to debian policy | ||
# https://www.debian.org/doc/debian-policy/ch-controlfields.html#version | ||
sanitized_docker=$( echo "${{ inputs.docker-image }}" | sed -r 's/[^a-zA-Z0-9.+~]+/+/g' ) |
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.
Ideally I'd use lsb_release -r -s
, but it has to run inside the target docker so that we have an incremental number (while docker names can be static or even cyclic).
sanitized_docker=$( echo "${{ inputs.docker-image }}" | sed -r 's/[^a-zA-Z0-9.+~]+/+/g' ) | ||
|
||
# Short commit to avoid "package-has-long-file-name" | ||
commit=$(echo ${{ github.sha }} | cut -c1-8) |
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.
Why don't we use git ourselves here?
So we can ahve something fancier by using echo git+$(git rev-list --count HEAD).$(git rev-parse --short HEAD)
(or similar), and we have also an incremental number together with the SHA.
export DEBFULLNAME="GitHub actions runner" | ||
export DEBEMAIL="[email protected]" |
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.
Or maybe latest commit author?
export DEBFULLNAME="GitHub actions runner" | |
export DEBEMAIL="[email protected]" | |
export DEBFULLNAME="$(git log -1 --format='%an' HEAD) - GH Action" | |
export DEBEMAIL="$(git log -1 --format='%ae' HEAD)" |
Plus an indication that is GH though, to make clear both things.
|
||
export DEBFULLNAME="GitHub actions runner" | ||
export DEBEMAIL="[email protected]" | ||
debchange --local "~${sanitized_docker}+${commit}" "Github build. Run id: ${{ github.run_id }}. Run number: ${{ github.run_number }}." |
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.
debchange --local "~${sanitized_docker}+${commit}" "Github build. Run id: ${{ github.run_id }}. Run number: ${{ github.run_number }}." | |
debchange --local "~${sanitized_docker}+${commit}" \ | |
"Github build. Run id: ${{ github.run_id }}. Run number: ${{ github.run_number }}." |
lintian: | ||
required: false | ||
description: Arguments to pass to lintian, if any. Set to false to skip the lintian chek. | ||
default: '' |
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.
This should be true
IMHO (to make it clearer at the reader as I see that by default it runs).
Edit: I saw why it's unset, but as explained later I think we're maybe giving too much control to action users :)
if: inputs.lintian != 'false' | ||
shell: bash | ||
working-directory: ${{ env.SOURCE_OUTPUT_DIR }} | ||
run: lintian ${{ inputs.lintian }} -- *.changes |
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.
run: lintian ${{ inputs.lintian }} -- *.changes | |
run: lintian --pedantic --fail-on error ${{ inputs.lintian }} -- *.changes |
So we still see what's not exactly clean. Plus, IMHO, it would be nice to have an option to control --fail-on
, since ideally good packages should set it to warning
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.
Oh, I sorry ignore me on the --fail-on
thingy... It's indeed possible to override the value.
Although, maybe this option is even too powerful, since IMHO any suppression should be in sources.
- name: Copy source package to output dir | ||
shell: bash | ||
run: | | ||
set -eu | ||
cp -r ${{ env.SOURCE_OUTPUT_DIR }}/* ${{ env.BUILD_OUTPUT_DIR }} |
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.
IMHO, we should do this phase just after building the source, and ensure that this happens also if binary build fails: This can help a lot to debug situations failure situations, because one can download the sources locally and build them with sbuild
(or similar tools) to fix the inconsistencies.
Also in commit message, we mostly spell it as "art_i_fact" even if both words seems to be correct.
run: | | ||
set -eu | ||
cp -r ${{ env.SOURCE_OUTPUT_DIR }}/* ${{ env.BUILD_OUTPUT_DIR }} | ||
- name: Upload artefacts |
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.
-> artifacts
@@ -79,6 +83,11 @@ runs: | |||
if [ -n "${GITHUB_TOKEN}" ]; then | |||
git config --system url."https://api:${GITHUB_TOKEN}@github.com/".insteadOf "https://github.com/" | |||
fi | |||
- name: Run lintian |
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.
Ideally this should run in the same distro in which we build the package... So I'm wondering if we should fork the jtdor/build-deb-action@v1 action to support post-build-hooks or just a run-lintian option.
@3v1n0 I'm a bit busy at the moment, feel free to take over this PR if you can and want. I think I'll be able to work on it in a couple of weeks otherwise. |
Yeah, nw... I've took something of this in #28 but we can revisit it. Also I didn't add the lintian thing for now as that can probably be split as another job so that could potentially be triggered in parallel with autopkgtests for example |
Closing as I will not have time to work on this. I'll keep the branch alive in case someone wants to continue it. |
…s and add tests (#28) As underlined in ubuntu/authd#130 (comment) using `jtdor/build-deb-action` for building debian packages has some problems that are yet not clear, and it also does not seem to properly support building non-native packages. As per this, given that the code needed for handling the build inside a simpler docker container is quite small, I've moved the build to manual handling which allows us some more control over the steps at the cost of being a bit more verbose. As bonus: - We now only build as user (not the sources, since there's no much point) - All the network traffic is totally blocked for the builder user (not just HTTPS) - ~~`DEB_BUILD_OPTIONS` can be customized for special builds (e.g. `nocheck`)~~ - Version uses actual distro version I've added some commits from #22, but not included the lintian build yet (that I've ready [here](3v1n0@1b72205)) not to make this PR harder to check, but also because I think those could instead be part of another action to allow more parallelization when used. By doing this change, not only both test cases pass, but also authd is correctly built: ubuntu/authd#130 (e.g. https://github.com/ubuntu/authd/actions/runs/8044158864) UDENG-2439
Being tested in canonical/ubuntu-pro-for-wsl#546
UDENG-2235