-
Notifications
You must be signed in to change notification settings - Fork 136
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
actions: debootstrap: Add parent-suite property to indicate which suite a downstream is based on #424
base: main
Are you sure you want to change the base?
Conversation
Would be great if you could add a test for this case |
38c4eb0
to
2bab6e7
Compare
as discussed, next step is to add some CI tests to build old/new debian/ubuntu/kali releases :-) |
2bab6e7
to
34532f0
Compare
cc @elboulangero for the Kali rolling tests. Is that good enough ? |
It's excellent, thanks very much for this work. Tested it, works as expected for Kali. |
34532f0
to
99fcb9e
Compare
99fcb9e
to
8eecdbb
Compare
Allow downstream distros to indicate which suite the bootstrapping should be done for. For now, only use this property to gate the debootstrap workaround for excluding usr-is-merged package to the parent suite which should be beneficial to downstreams. Fixes: go-debos/debos!361 Signed-off-by: Christopher Obbard <chris.obbard@collabora.com>
Allow the debootstrap script to be set by the user to a full path. If unset, use the existing behaviour of using the unstable debootstrap script. Signed-off-by: Christopher Obbard <chris.obbard@collabora.com>
…suite If the debootstrap script property is unspecified, set the script to be the suite property, falling back to the parent suite if the script doesn't exist and finally falling back to unstable if the parent suite doesn't have a custom script. Signed-off-by: Christopher Obbard <chris.obbard@collabora.com>
a70647d
to
7b53bef
Compare
Kali rolling is based on Debian unstable. Add a test to ensure that we can build downstream distros with a different suite name to upstream Debian. Signed-off-by: Christopher Obbard <chris.obbard@collabora.com>
Apertis v2023 is based on Debian bookworm. Add a test to ensure the parent-suite logic works to build downstream distros with a different suite name to upstream Debian. Signed-off-by: Christopher Obbard <chris.obbard@collabora.com>
Ensure that we can build current debian stable & testing. Signed-off-by: Christopher Obbard <chris.obbard@collabora.com>
7b53bef
to
7b6ce52
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.
lots of questins; need some pondering if this is the right shape
cmdline = append(cmdline, "/usr/share/debootstrap/scripts/unstable") | ||
|
||
if len(d.Script) > 0 { | ||
if _, err := os.Stat(d.Script); err != nil { |
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 done in verify not at run time
@@ -136,6 +136,8 @@ jobs: | |||
test: { name: "arch", case: "arch" } | |||
- backend: kvm | |||
test: { name: "apertis", case: "apertis" } | |||
- backend: kvm | |||
test: { name: "kali", case: "kali" } |
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.
Does kali not build without this? Fwiw i'm wary about adding rolling releases as they might break CI too easily
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.
Hello Sjoerd,
Does kali not build without this
Nope, it doesn't, since it's based on Debian testing. I tried with latest debos from the Docker Hub, just to be sure:
$ cat recipe-kali.yaml
architecture: amd64
actions:
- action: debootstrap
mirror: http://kali.download/kali/
suite: kali-rolling
components: [ main ]
keyring-file: kali-archive-keyring.gpg
keyring-package: kali-archive-keyring
- action: apt
packages: [ procps ]
$ podman run --rm -it --device /dev/kvm --workdir /recipes \
--mount "type=bind,source=$(pwd),destination=/recipes" \
--security-opt label=disable \
godebos/debos:main-20240113 recipe-kali.yaml
Running /debos --artifactdir /recipes /recipes/recipe-kali.yaml using kvm backend
2024/01/15 04:49:58 ==== debootstrap ====
2024/01/15 04:49:58 excluding usr-is-merged as package is not in suite
2024/01/15 04:49:58 Debootstrap | I: Retrieving InRelease
[...]
2024/01/15 04:51:41 Debootstrap | I: Base system installed successfully.
2024/01/15 04:51:42 ==== apt ====
2024/01/15 04:51:42 apt | Hit:1 http://kali.download/kali kali-rolling InRelease
2024/01/15 04:51:43 apt | Reading package lists...
2024/01/15 04:51:43 apt | Reading package lists...
2024/01/15 04:51:43 apt | Building dependency tree...
2024/01/15 04:51:43 apt | procps is already the newest version (2:4.0.4-2+b1).
2024/01/15 04:51:43 apt | You might want to run 'apt --fix-broken install' to correct these.
2024/01/15 04:51:43 apt | The following packages have unmet dependencies:
2024/01/15 04:51:43 apt | init-system-helpers : Depends: usrmerge but it is not going to be installed or
2024/01/15 04:51:43 apt | usr-is-merged
2024/01/15 04:51:43 apt | E: Unmet dependencies. Try 'apt --fix-broken install' with no packages (or specify a solution).
2024/01/15 04:51:43 Action `apt` failed at stage Run, error: exit status 100
I suppose all derivatives based on Debian testing/sid are affected.
Note that we can (and we do) workaround by adding this just after the debootstrap step:
- description: "Install usr-is-merged (cf. debos #361 and #362)"
action: apt
packages: [ usr-is-merged ]
So it's not a dealbreaker, we can live without the MR. But it's nicer when we don't have to use cryptic workarounds.
i'm wary about adding rolling releases as they might break CI too easily
Debootstrapping Kali rolling is just like debootstrapping Debian testing, only two packages in the set are forked, the rest is just Debian testing. Usually kali-dev
(more or less equivalent to Debian sid) breaks every now and then, but we have solid QA that keeps kali-rolling
working.
You might want to use the mirror kali.download
instead of http.kali.org
, so you'll hit Cloudflare CDN rather than our redirector, for maximum reliability.
Thanks
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.
I'd like to keep the Kali test in - with the mirror change @elboulangero mentions above.
@sjoerdsimons are you happy with that ?
- { name: "debian (bookworm armhf)", case: "debian", variables: "-t architecture:armhf -t suite:bookworm" } | ||
- { name: "debian (trixie amd64)", case: "debian", variables: "-t architecture:amd64 -t suite:trixie" } | ||
- { name: "debian (trixie arm64)", case: "debian", variables: "-t architecture:arm64 -t suite:trixie" } | ||
- { name: "debian (trixie armhf)", case: "debian", variables: "-t architecture:armhf -t suite:trixie" } |
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 massively extends the number of test jobs; Does testing trixie on arm64/armhf give us extra data? Also i'm a tad wary about it breaking CI non-deterministically
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.
as was done to tune our runs; for trixie for now please just build amd64 and only on kvm
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.
yeah probably best to remove trixie
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.
So spending a bunch of time today thinking about it and remembering what the issue is again for background
Fundamentally the main part of this problem happens because the newer debootstrap scripts for Debian forced installing usr-is-merged (and to not install usrmerge), unless it detected a known "old" debian suite. Note that this didn't generally impact e.g. Ubuntu, tanglu or trisquel (as their debootstrap scripts don't do that).
Fundamentally the fix we did in debos was to exclude usr-is-merged unless we know the bootstrap distro was a newer debian to not break existing image builds for stable debian releases. For extra fun this never impacted apertis v2024 (bookworm based) as its init-system-helpers hard depend on usr-is-merged and the --exclude
switch only applies to the initial package selection not on the expanded dependency list (cause usr-is-merged to be installed on bootstrap in apertis even though it was excluded). On Debian >= bookworm and some derivatives init-system-helpers depends on usrmerge | usr-is-merged
, which with the exclusion by both debos and the debootstrap scripts meant neither got installed hence the issues :).
However it should be noted that this kind of gymnastics from debootstrap is exceedingly rare as should be debos doing these workaround.
Slightly orthogonally debos always forced the Debian debootstrap scripts (unstable specifically but they're all the same for any recentish debian release). Even though e.g. kali, ubuntu, tanglu, etc do have their own custom (to a smaller or bigger extend) debootstrap scripts.
So overall i think the step forward for debos should be to be able to use more appropriate debootstrap scripts (e.g. the ubuntu ones for ubuntu); e.g. either matched on the suite (if it's has it's own script directly) or by a property that indicates which suite to use by debootstrap (usually the direct parent, but there could be a longer lineage) or for more custom handling a deboostrap script as part of the recipe (for those needing a custom scripts, but don't have it available in debootstrap).
Most of that matches the patches you've already done; Though to paint some bikesheds:
- Change
parent-suite
intodebootstrap-suite
instead, with the semantic that the configured argument is what "suite" to use for debootstrap purposes (which primarily selects the right "builtin" debootstrap script but optionally does workarounds). - Change
script
intodebootstrap-script
with the argument being the path to the debootstrap script inside the recipe origin rather then a full path.
Having both a debootstrap-script
and debootstrap-suite
should cause an error as it makes no real sense. If you use a script and need workarounds they should be done in the custom script instead at which point the debootstrap-suite
setting has no meaning anymore :)
Also at the vary least if the suite
has a builtin debootstrap script then having a debootstrap-suite
should at the very least be a warning but i'd even error on the side of it being an error as it's both confusing and a very weird situation.
Having a debootstrap-script
in that situation should not be as problematic as e.g. someone is building a variant not supported in the debootstrap scripts. But the output should clearly indicate a custom script is used (probably in the debootstrap output)
For the current behaviour; we should use the current default "unstable" script as always (but also indicate the fallback in the output and potentially suggest to user to set debootstrap-suite
) if suite
doesn't have it's own script and neither of the new properties were given. In other words automatic fallback only happens if the user didn't explicitly ask for a specific suite or script for debootstrapping
For the usr-is-merged workaround we should only apply it when we detect a known old distribution (based on the suite used debootstrapping) but assume anything not specifically recognized doesn't need this workaround. This is similar to how debootstrap does this (though it only goes from the remote codename rather then the script name) and will be more future proof.
Your current patchset already gets quite closed to this so the changes should be minimal. For e.g. kali that means there is no reason to set a debootstrap-suite
as it already has its scripts in debootstrap and we'll pick those up now. For apertis v2024 the same goes (though building will correctly suggest it should be for consistency), while apertis v2023 (and older) now need a debootstrap-suite
setting to build image on a newer debian base/debos, but I think that's fine.
func (d *DebootstrapAction) isLikelyOldSuite() bool { | ||
switch strings.ToLower(d.Suite) { | ||
// Check if suite is something before usr-is-merged was introduced | ||
func shouldExcludeUsrIsMerged(suite string) bool { |
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.
We should flip this function around so the default is false
as it won't be needed for newer suites (whose set will increase) but will be for older debian ones (whose set won't change).. It also should default to false for newer debian derivatives (using >= bookworm) .
fwiw debootstrap has the following list: etch*|lenny|squeeze|wheezy|jessie*|stretch|buster|bullseye
no need to deal with jessie*
though given it is completely unsupported since 2020
- parent-suite -- release code name which this suite is based on. Useful for downstreams which do | ||
not use debian codenames for their suite names (e.g. "stable"). |
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.
Fwiw this could be used for non-debian parents as well.. E.g. parent-suite
could be jammy
for ubuntu derivatives (or kali for kali derivatives). We should require the parent-suite
to be known by debootstrap (as in a script exists) if given. (potentially this requirement should only exist if there is no script for suite
, but in that case this property is useless).
- { name: "debian (bookworm armhf)", case: "debian", variables: "-t architecture:armhf -t suite:bookworm" } | ||
- { name: "debian (trixie amd64)", case: "debian", variables: "-t architecture:amd64 -t suite:trixie" } | ||
- { name: "debian (trixie arm64)", case: "debian", variables: "-t architecture:arm64 -t suite:trixie" } | ||
- { name: "debian (trixie armhf)", case: "debian", variables: "-t architecture:armhf -t suite:trixie" } |
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.
as was done to tune our runs; for trixie for now please just build amd64 and only on kvm
No description provided.