Skip to content

feat(build-debian): Use docker to manually run build and prepare steps and add tests#28

Merged
3v1n0 merged 23 commits intocanonical:mainfrom
3v1n0:build-deb-docker
Nov 13, 2024
Merged

feat(build-debian): Use docker to manually run build and prepare steps and add tests#28
3v1n0 merged 23 commits intocanonical:mainfrom
3v1n0:build-deb-docker

Conversation

@3v1n0
Copy link
Copy Markdown
Contributor

@3v1n0 3v1n0 commented Feb 26, 2024

As underlined in canonical/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) 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: canonical/authd#130 (e.g. https://github.com/ubuntu/authd/actions/runs/8044158864)

UDENG-2439

Copy link
Copy Markdown
Contributor

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

I have to stop the review for now here, already posting what I found.

This is clearly post FF material…

@3v1n0 3v1n0 force-pushed the build-deb-docker branch 2 times, most recently from b574a4e to 0a2059d Compare February 26, 2024 13:22
@3v1n0
Copy link
Copy Markdown
Contributor Author

3v1n0 commented Feb 26, 2024

This is clearly post FF material…

Yeah, in theory it is, but I liked the idea to have canonical/authd#223 in before and so being tested too.

@didrocks
Copy link
Copy Markdown
Contributor

I propose we focus on the FF itself, remove the deb building part, polish the packaging, release and then, we can go over that one.

@3v1n0 3v1n0 force-pushed the build-deb-docker branch 2 times, most recently from 524fee0 to 6f061b8 Compare February 26, 2024 15:10
@3v1n0 3v1n0 force-pushed the build-deb-docker branch 2 times, most recently from cdd3e9e to d46aa33 Compare October 17, 2024 14:29
@3v1n0 3v1n0 requested a review from didrocks October 17, 2024 14:30
Copy link
Copy Markdown
Contributor

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Can you make test and ensure that WSL pro service still works with it? After that, I’m happy to get it merged. I didn’t spot anything else to change.

@3v1n0
Copy link
Copy Markdown
Contributor Author

3v1n0 commented Oct 17, 2024

Can you make test and ensure that WSL pro service still works with it

You mean prepare a PR for that? As tests are included in this branch already

@didrocks
Copy link
Copy Markdown
Contributor

I mean, we have one known consumer at this point (https://github.com/canonical/ubuntu-pro-for-wsl/blob/main/.github/workflows/qa-azure.yaml#L30): let’s double check that the changes here doesn’t impact negatively it as a double check in addition to the local test!

@3v1n0 3v1n0 force-pushed the build-deb-docker branch 2 times, most recently from 8d67413 to c59b4bd Compare November 12, 2024 23:46
Parse the tools using jq so that we can easily get the tool path and
version and install the one explicitly requested
didrocks and others added 22 commits November 13, 2024 00:51
Fix warnings related to:
- Version number
- File too long
- No contributor name
- Extra whitespace around name
We don't fully support source builts for non-native packages yet though,
so ignore the error for now
Artifacts could be exposed by various other composed jobs so make this
clearer for binary packages too
This allows us to have more control on the volumes we mount in
docker and on the location of the source path.

And permits us to properly mount the parent folder of the source
that we need to access to read the .orig files for non-native
packages.

It comes as a little extra cost, but also it give us way more
control on the builder instance.
In general when building sources we should not need anything else a part
what is in build-depends, but for some packages we may need some extra
packages such as ca-certificates or git.

So, expose these values as extra-source-build-deps input.

This option should actually be unset by default, but we don't yet not to
break existing packages, but we should change it ASAP so that it depends
on each repo needs.
It may be useful for debugging purposes, so upload it once it's ready
For some reason using jtdor/build-deb-action@v1 does not work well
with some rust builds we have, so re-implement it using some manual
labor. It's not really much work and it will allow us more control.

Other than, working authd builds.
When building debian packages we should not have high privileges, so
let's just use an unprivileged user to perform the binary build.

It's not too needed for source builds, since those are not really
doing tests or anything where having root privileges may lead to
different behavior
Ensure that internet access is completely disabled for builder user
when creating the binary package.

This is something that we somewhat had implicitly for HTTPS only since
ca-certificates were not installed.

But let's ensure this in any case.
It allows to perform special builds (such as nocheck ones), so expose
it in case, for example, one may want to build without running tests.
In this way we can use the proper distro version number
In case a git repository is setup, then use version number that
includes the revision list and use actual author name
It can provide some quick information about what's in the packages
Use a +git syntax and there we use incremental version values.

Finally add the distro version as a minor factor, so that builds on the
same code for different distros only rely on that
Since we're building as user
@3v1n0
Copy link
Copy Markdown
Contributor Author

3v1n0 commented Nov 13, 2024

I mean, we have one known consumer at this point (https://github.com/canonical/ubuntu-pro-for-wsl/blob/main/.github/workflows/qa-azure.yaml#L30): let’s double check that the changes here doesn’t impact negatively it as a double check in addition to the local test!

Ok, so using 3v1n0/ubuntu-pro-for-wsl@53e71d8 in my fork worked properly to build the deb and source files (after few adjustments), so let's get this in.

@3v1n0 3v1n0 merged commit 6e15fc5 into canonical:main Nov 13, 2024
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.

3 participants