feat(kernel,initrd): extend plugins with new options, stabilize#6254
Conversation
Error within the `if` block, as otherwise we should do the default thing. The way it was written, if we didn't specify a release but also didn't specify a source, the pull step fails. Signed-off-by: Dilyn Corner <dilyn.corer@canonical.com>
Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
We've already got it, might as well use it. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Use the more canonically accepted kernel file name. This frees up kernel.img to be used for other things without causing unecessary confusion, as kernel.img have either been the kernel file itself or the UKI EFI binary. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
… arches By popular demand. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
26.04 saw cdimage's structure change, so tarballs are in different locations depending on the release. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
The files for checking what phase of the initrd build had ocurred was only loosely utilized. This change makes it more explicit. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
This should keep the initrd and kernel in their appropriate lanes during builds. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
The behavior was backwards. Now it's correct. We don't necessarily want cleaning to force failure if umounting fails; the path could be unmounted for a variety of reasons. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
dirmngr and gpg-agent love to hold on *forever*. Don't let them. Cleaning actually cleans, now. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
pid in this case resolves to /proc/xxxx, which is invalid usage of kill. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
`echo` doesn't properly strip spaces in our variables like we need it to; since the values are all already on new lines anyways we don't need to iterate in a loop anyways, and we can just let IFS split our list and iterate over that directly. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Because initrd_efi_image_{key,cert} have default values, the XOR check was always false.
Instead, the check must consider whether a value different from the default has been
set, and if one has been updated and not the other we should fail.
Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Should improve error readability, which is important in this context as the user would be the one making the mistake. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
The check in get_pull_commands is never actually hit, as commands.extend() will always populate the pull commands with *something*. The failure will happen when the pull step is actually executed. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
OLDPWD is previously set in a different function, and it's potentially possible that build_tool could inherit this value instead of the correct one. To avoid the possible (but very unlikely) edge case, make OLDPWD more explicit. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
This option supports building the kernel and its associated packages based on the debian/rules makefile. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
It's possible for a user to specify: initrd-addons: - foo/bar/ initrd-firmware: - firmware/baz/ In these cases, the path stripping will resolve these items to "", and the whole process will silently fail. Trailing slashes should be stripped before the process begins to handle this case. Multiple trailing slashes are unlikely to occur in this context, so they are not handled. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
There are cases where custom configs should still be allowed, like with kernel-ubuntu-debian-package. Kconfigs in that context would be annotations fragments, and need to be handled in a specific way. The docs have been clarified to be a bit more explicit with the expectations associated with some options. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
This improves the readability of the primary function; now, most behavior is sequestered into their own sections. Traceability is still a little challenging, but at least the logic is easier to follow now. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Instead of passing comma-separated lists and parsing, we can pass quoted lists to each argv and interpret in the "usual" sense. Drops many seds, and use tr where appropriate. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Should be easier to grok for the general reader. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Extend the behavior of building debian package kernels to suppor the more general use-cases commonly used by those building kernel debian packages. The upshot is supporting DKMS packages. Also resolves some small bugs or inconsistencies. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Leverages the flexibility PartsErrors allows us and give suggestions to the user for what to do instead. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
run() can be difficult to follow if you're not familiar with all the moving parts. Include some explanation of how various options impact the logical flow of each plugin. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Reflects current state of changes. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
We always want to blow away any debs in this context. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
|
Not that I have approval right, but I tested this with multiple of my custom kernels (build base |
bepri
left a comment
There was a problem hiding this comment.
Approving despite the many test failures, because:
- Lint, OSV and integration test failures will be fixed when main is merged into this branch
- Initrd/Kernel spread test failures are flaky, as confirmed by Dilyn via MM
- Other tests are also, rightfully, marked as flaky in the first place.
It's quick enough that it doesn't need to be multithreaded and there's the potential for race conditions, especially in more recent kernels (6.18 has such an issue). Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Most changes amount to grammar, phrasing, or clarity. Some tests have been improved or added to the run list. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Use spread once. Tests should provide the message to clarify their meaning. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
heredocs are fragile when embedded in yaml; to avoid annoyance, switch to some echos. kernel test now checks the appropriate locations for built tools. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
and improve kernel tests. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Avoid the nasty trickery trying to fetch cross-packages on Jammy and earlier. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Switching to build-base: core24 means that we use platforms key instead of architectures, and so the final snap package name is different. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
This test will probably never really succeed in CI due to requirements in the build environment; we need binfmt_misc support in LXD to run the ARCH-specific binaries in the chroot. What really matters is that the premise works, and the native test achieves that goal. If cross-building fails, it's probably for a reason unrelated to the plugin itself. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
cdimage has been updated so that resolute follows the same convention as other releases. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
On amd64, the CPU microcode packages get removed if we rm -rf
${INITRD_ROOT}/lib/firmware *after* we configure the chroot, because
chroot_configure() installs the microcode packages! Clean firmware,
modules first. Then configure.
Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
medubelko
left a comment
There was a problem hiding this comment.
@dilyn-corner Glad to see this finally land! Approving pre-emptively. All of my suggestions are about languages quirks.
|
|
||
| A list of DKMS packages to include in the debian package build of the kernel. | ||
|
|
||
| This option is only meaningful when used with ``kernel-ubuntu-debian-package``. |
There was a problem hiding this comment.
| This option is only meaningful when used with ``kernel-ubuntu-debian-package``. | |
| Requires the ``kernel-ubuntu-debian-package`` key to be set. |
There was a problem hiding this comment.
set or true? It's a bool, so setting it is equivalent to key to be 'true', but I'm uncertain of the vernacular you're establishing with your audience.
There was a problem hiding this comment.
Similar to the "non-nil" question from earlier. "is/if set" and "is/if unset" is meant to succinctly communicate "if this key is declared in your project file".
If the key must indeed be true, then it should read something like
Requires the
kernel...key to be set totrue.
Or:
This key only takes effect if the
kernel...key is set totrue.
Config values are tricky to write about, because they have three states:
- Not assigned by the user, and with no default value
- Not assigned by the user, with a default value (most often true/false, but non-bool and procedural values are also possible)
- Assigned by the user
The result is that asserting any value and asserting no value are important distinctions for the user. In terms of guidance, we need succinct language to communicate a user's inaction (unset) in and action (set).
There was a problem hiding this comment.
That all makes sense. I suppose I took the position of documenting deviations from default behavior, which impacts the phrasing of things like this; kernel-ubuntu-debian-package is an "optional" key, and isn't present in a project file unless the user intentionally adds it; because it's a bool, it couldn't be set to anything else, and so it being "set" is equivalent to it being "True".
I can always be more explicit, though. Indeed, I am explicit about this in other places (line 56, for instance). So I'll change this to match:
Requires the ``kernel-ubuntu-debian-package`` key to be ``true``.
| kernel snap. | ||
|
|
||
| This option is the fastest route to producing a kernel snap when a kernel debian | ||
| package already exists, and no real changes need to be made to it. |
There was a problem hiding this comment.
"real" is a subjective qualifier. Could we be more concrete?
There was a problem hiding this comment.
"real" in this case means something more like "actual" -- what I'm trying to get at is that every other option (well, most) is intended for the intensive case, where one needs to make changes to the options enabled at kernel build time. This route is special, in that you're fine with how the kernel was built, you just want a snap instead. So I think 'actual' captures that?
There was a problem hiding this comment.
What would it mean if we wrote "no changes need to be made" or "the kernel Debian package is intended to be used as-is"?
There was a problem hiding this comment.
I think either option would be equivalent to what I am trying to say; I'll go with the second phrasing.
Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
Replace the legacy function with the modern one. Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
1dbc267 to
6494c40
Compare
Signed-off-by: Dilyn Corner <dilyn.corner@canonical.com>
|
@dilyn-corner @bepri Doc updates look good! |
|
Override-merging for the reasons stated above, these failures are either expected (mostly 'cuz flaky) or will be resolved once |
518476f
into
canonical:feature/kernel-initrd-plugin
This merges `main` into the feature branch. Requires #6254.
This PR extends the behavior of the kernel plugin to include features desired by the Canonical Kernel team. While these options are intended for that specific usage, they are valid options to use in other use-cases.
Their inclusion has expand the complexity of the kernel plugin, so documentation has been improved to try and clarify the most salient choices which impact the decisions snapcrafters should make when crafting a kernel snap. A new spread test has been added (kernel-deb, and the old kernel-deb test has been renamed kernel-bin) to cover this case in a sufficiently comprehensive way.
Comments have been added to the kernel build script to identify the primary patterns of behavior the script will execute.
Test coverage has been expanded to cover the new options.
The initrd plugin's behavior has been modified a bit to improve usability and stability. It should be less fragile now, especially when it comes to cleaning the environment.
The overall behavior and stability of the plugins has been improved.
These changes have been tested by Field and the Kernel team has discussed them (I will not speak for them, we can always ask).
All spread and unit tests pass locally. Docs lint and render well :)
Any behavior-specific changes (like naming the installed kernel image file
vmlinuzinstead ofkernel.img) have been debated internally so all stakeholders are aware of these things; the impact should be minimal as most users are those same stakeholders.A caveat: with the release of Resolute, a change was made in the location and name of the Resolute release tarball used by the initrd plugin. I have been informed that this is only a short-term divergence and the prior naming convention will be reused at some point. This means that there will be a future PR to fix the initrd plugin once this happens. I have no timelines for this, but I have setup daily tests to check this URL :)This has been resolved.This PR precedes #6255, which updates the feature branch to
HEAD.The line count is deceptive; the new test adds a whole kernel config file. This alone is over 4k lines.
CC @jicheu @atomcult @kubiko @stewarthore
make lint && make test.