build only the packages needed for the current variant #1408
Conversation
|
I'll do some squashing and try to figure out the best thing for the kmod kit target today. |
This squashes things into a reasonable set of commits. TODOs
|
Fix an errant lock file. |
Builds the kernel as a dependency of build-kmod-kits, which previously relied on less specific prerequisites. |
Remove os from the variant dependency lists since it is a dependency of release. (Leaning on CI to prove it.) |
Separate package dependencies between |
Fix a couple of mistakes in the |
|
Added a commit to remove |
|
One of the side effects of this change is that modifying certain packages (systemd) will trigger many more package rebuilds than they previously did. It might be worth exploring the graph with I'd say we might want to consider removing containerd's dependency on systemd, and the kubelet + docker-engine dependencies on containerd. We know both will end up on the final image by way of the release package, and they aren't needed at build time. |
A rebase that git was surprisingly angry about and therefore might need more work. We'll see. |
|
I'm going to switch this back to draft for now since my next push will be somewhat of a proposed solution. |
|
After a push that didn't work out, then a revert... this push takes care of a surly rebase over develop
|
|
It seems the GitHub UI doesn't show the fact that I just pushed a new commit: 0caaf138909e83b84a7c264083523f725c133e51 In this commit I combine package and variant workspaces into a single workspace so that they have a shared target directory. This should solve the problems @bcressey detected here #1408 (comment) There's something no one is going to like... I had to
So for now what I have pushed is something that would work if we could live with the moving of the packages directory. |
In this version packages are no longer members of a workspace. This allows us to leave the packages directory alone instead of moving it into the variants workspace. |
|
@bcressey this is ready for re-review. I think there might be more you wanted to do with moving some deps from the variant's cargo.toml to common release.spec dependencies. But everything else is done. |
Remove the individual variant lock files since they are part of a workspace now. |
|
rebase oops, unsquashed a couple of fixup commits |
resquash a couple of fixups |
|
Failed builds, I guess I need to add host-ctr to os's Cargo.toml. |
Add host-ctr to os Cargo.toml 🤞 |
Fix Cargo.lock files in all affected commits 🤞 |
Add bootstrap-containers to the os/release package dependency refactor. Edit: oops bad push |
undo bad push |
Add bootstrap-containers to the os/release package dependency refactor (without undoing other work this time). |
Very minor rebase |
Attempt at README correctness. |
| ``` | ||
|
|
||
| The [package.metadata](https://doc.rust-lang.org/cargo/reference/manifest.html#the-metadata-table-optional) table is ignored by Cargo and interpreted by our `buildsys` tool. | ||
|
|
||
| It contains an `included-packages` list which specifies the packages to install when building the image. | ||
| In the `[build-dependencies]` section, we specify the packages that need to be built, which is sometimes slightly different than `included-packages`. | ||
| This populates the Cargo build graph with all of the RPM packages that need to be built before the variant can be constructed. |
There was a problem hiding this comment.
I'm finding these new sentences a bit confusing. build-dependencies are rather different than package.metadata, but they're stuck in the middle of that paragraph - can we separate it? (Unless they're always intended to match, in which we should probably make less of a distinction.) It also has the effect of making it unclear whether the sentence just after this about including the release package applies to included-packages, build-dependencies, or both. (Either way... why?)
And I have to ask... if they need to match, can we get rid of included-packages and have buildsys read the names of the build-dependencies instead? I see there are a couple package names that differ, but I bet we can come up with a solution for that rather than entirely duplicating it. For example, I just tested and found that you can add extra keys to dependencies for your own purposes; we could do something like chrony = { path = "../../packages/chrony", included_rpms = "chrony-tools" } for those special cases.
I'm not sure we need the technical wording here about the build graph, unless it's important to explain the difference between included-packages and build-dependencies. I think the audience for this doc is someone trying to build a variant, not work on the build system.
There was a problem hiding this comment.
What do you mean by satisfied by the current package
For example os.spec Requires: apiserver, but os.spec defines/builds the apiserver package, so there is nothing to add to [dependencies].
if they need to match, can we get rid of included-packages and have buildsys read the names of the build-dependencies instead
That seems like a reasonable enhancement to consider. I think it should be outside of the scope of this PR, but we could open an issue.
packages/README.md
Outdated
| With build-dependencies (`BuildRequires:`) it's important that they be built *before* the current package is built. | ||
| Runtime dependencies (`Requires`) can be built at any time. | ||
| The separation between `build-dependencies` and `dependencies` in Cargo.toml is not necessary but helps us distinguish between the two dependency types. | ||
| Note that we do not include `Requires:` dependencies if they are already satisfied by either the current package, or the build-dependencies. |
There was a problem hiding this comment.
I'm not sure I understand this. What do you mean by satisfied by the current package?
I think the biggest potential confusion we have is from someone adding or updating a package who doesn't understand what needs to go in Requires vs. dependencies, BuildRequires vs. build-dependencies, and dependencies vs. build-dependencies. Several comments here imply that dependencies = Requires, then we walk it back a bit here; I think it could be clearer to remove the "# RPM Requires" comments in the Cargo.toml files (since they're not really true) and have people read this doc instead, and in this doc, also remove the equivalence statements above, and have one section that clearly explains what [dependencies] should be and how/when they differ from Requires.
Part of the confusion is that we don't list all of the Requires as dependencies, even if they're not satisfied by build-dependencies, in special cases where we just know it'll cause problems. (unnecessary systemd rebuilds, glibc, etc.) If that's always going to be a small set, we could just list them, but either way it's probably worth mentioning.
There was a problem hiding this comment.
I think it could be clearer to remove the "# RPM Requires" comments in the Cargo.toml
The closest thing we have to a mapping is: Requires: -> dependencies and BuildRequires: -> build-dependencies. The mapping is true even if some are omitted.
I definitely think it's more helpful than not to have the # RPM Requires comment in Cargo.toml even if you need the README to fully understand what's going on.
I'll take another stab at explaining it better in the README.
| [dependencies] | ||
| cni-plugins = { path = "../cni-plugins" } | ||
| runc = { path = "../runc" } | ||
| systemd = { path = "../systemd" } |
There was a problem hiding this comment.
I'm having a hard time following the discussion at the top level of the PR, but are we sure we still want to include "global" [dependencies] like systemd that would be unnecessarily rebuilt? Per #1408 (comment)
There was a problem hiding this comment.
are we sure we still want to include "global" [dependencies] like systemd that would be unnecessarily rebuilt?
My preference would be to go ahead with the current version which does not have the global dependency removal optimization. It's a two-way door, but it seems better to me to start with fewer exceptions.
Fixes to readmes and missing dependencies. |
rebase |
Remove k8s-1.15 from variants workspace. |
Signed-off-by: Ben Cressey <[email protected]> Change os.spec such that all first party code is installed when the os package is installed. Change release.spec such that it requires only the os package for first-party code instead of requiring each individual first-party package. This consolidates the package dependencies for first-party code into one place, instead of picking some of the packages in "release" and others through the variant definition. This is part of #1361. We want to better manage package dependencies with cargo so that we can build only the packages needed for a given variant, and this change eliminates some one-off first-party package dependencies that needed to be expressed at the variant level.
rebase (k8s 1.15 package removal) |
Previously we expressed RPM BuildRequires dependencies in Cargo's dependency graph to ensure the necessary RPM's exist before we build a package. If we add to this RPM Requires dependencies (dependencies that are needed when software packages are installed), then we can use Cargo to select only the packages that are needed for a set of desired install packages.
Adds the packages that are needed to build a variant to the variant's Cargo dependencies. Previously the build-variant makefile target assumed that all packages were pre-built. Instead we now tell Cargo to build the packages we need for the variant. Creates a workspace in the variants directory and removes the workspace in the packages directory. Updates the makefile accordingly. Now only the packages that are part of the dependency tree starting with the variant will be built. The build-kmod-kit target previously depended on build-packages, which is no longer available. So a new target, build-package, and a specialization of it, build-kernel, have been added. These use the same workspace context that is used when building variants to ensure that the same rpm is used by build-kmod-kit.
cargo make world targets were vestigial. Probably nobody is using them so we cleaned them up.
Another stab at explaining dependencies in packages/README |
Issue number:
Closes #1361
Description of changes:
Utilize cargo dependency graph to build only the packages we need for the current variant.
Packages express a dependency on anything that the rpm spec file declares a dependency on. (We already did this for
BuildRequiresbut now we do it forRequiresas well.) Additionally, the variant'sCargo.tomlneeds to express the things the package crates it depends on.Note: We now rely on a change in behavior in cargo 1.51.0 to allow us to build a package that is a dependency of a workspace, but which is not a workspace member. You will need to
rustup updateif you have an older version.Testing done:
Starting from a clean build state:
cargo make -e BUILDSYS_VARIANT=aws-k8s-1.18cargo make -e BUILDSYS_VARIANT=aws-k8s-1.19cargo make -e BUILDSYS_VARIANT=aws-k8s-1.18 repocargo make -e BUILDSYS_VARIANT=aws-k8s-1.19 repofind . -type d -name target./tools/target./variants/targetStarting from a clean build state:
cargo make build-kmod-kitfind . -type d -name target./tools/target./variants/targetStarting from the above kmod-kit build state:
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.