implement a dependency graph algorithm for package and kit builds #198

webern · 2024-04-13T00:54:18Z

Issue number:

Closes #185

Description of changes:

For kit builds we are going to need to understand the dependency graph in buildsys build-kit. We may need it for other operations as well.

This PR changes buildsys so that it requires a cargo-metadata json file which is uses to understand the dependency graph and send the list of package dependencies and kit dependencies to the Dockerfile in build args.

This implementation requires that Twoliter projects be organized as in a single workspace with all packages, kits and variants in the same Cargo.toml (which feels like an improvement overall).

Testing done:

unit-testing on the in-tree test project local-kit
Build Bottlerocket (with changes to make it into one workspace): webern/bottlerocket@8933289...webern:bottlerocket:workspace
Build an AMI
Rebuild the image to check incremental compilation (only os , release and the variant should be rebuilt) - (aws-iam-authenticator was also rebuilt, but that doesn't seem show-stopping)

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.

tests/projects/project1/packages/hello-agent/Cargo.toml

tests/projects/project1/packages/hello-go/Cargo.toml

tools/buildsys/src/deps.rs

webern · 2024-05-01T18:44:06Z

https://github.com/bottlerocket-os/twoliter/compare/0e06bb6c06f354db5816821b215641906755982d..c150a5584c3fe542705869d4d7cd8a16864315d7

Rebase

webern · 2024-05-02T00:38:39Z

https://github.com/bottlerocket-os/twoliter/compare/c150a5584c3fe542705869d4d7cd8a16864315d7..665c8598b6ed8c9deb4fb58f5a5c9d54e29565b5

Now using Guppy and getting a bit closer to where we are going with this.

tests/projects/local-kit/packages/pkg-d/Cargo.toml

tools/buildsys/src/deps.rs

webern · 2024-05-04T00:29:53Z

Current status: encountered during a Bottlerocket build at the filesystem package:

  --- stderr
  Unable to instantiate the builder: Failed to create build arguments due to a dependency error: Expected to find one of build-package, build-kit, or build-variant in package.metadata.

Turns out that Cargo.toml doesn't have build-package in it. I will fix it and continue on Monday

[package]
name = "filesystem"
version = "0.1.0"
edition = "2021"
publish = false
build = "../build.rs"

[lib]
path = "../packages.rs"

webern · 2024-05-05T22:03:40Z

Over the weekend I made some changes to the Bottlerocket diff and the testing passed. This is ready for review.

webern · 2024-05-08T17:05:58Z

https://github.com/bottlerocket-os/twoliter/compare/596002d362144e6b941596234301c927ad38f061..e5ee4e19e43c322a92613c73fd1cfe7e8bf54d40

Improvement to the algorithm. For package build, for the top-level manifest, we should only consider build-dependencies. When lower in the tree/graph, we should consider both build-dependencies and dev-dependencies.

Twoliter projects, and kit builds in particular, will benefit from having a single workspace that includes all packages, kits and variants.

The snafu errors in Buildsys/main.rs were printing the enum value only without any underlying error information,

bcressey · 2024-05-08T19:45:03Z

tests/projects/local-kit/kits/extra-1-kit/Cargo.toml

-included-packages = [
-    "pkg-b",
-    "pkg-d",
-]


Are we not doing included-packages for kits any more?

It might be more natural to support an optional excluded-packages instead, if the default should be that all packages go into the kit.

An included-packages would be sort of gnarly to implement since it would need to inform the graph algorithm. When we talked about it we were thinking we would just go with Cargo dependencies defining the package list. excluded-packages could be a feature add fairly easily if anyone ends up needing that.

twoliter/embedded/Makefile.toml

bcressey · 2024-05-08T20:15:41Z

tools/buildsys/src/args.rs

+    assert!(BuildFlags::Package.includes(PACKAGE | VARIANT));
+    assert!(BuildFlags::Variant.includes(VARIANT));
+    assert!(BuildFlags::Variant.includes(VARIANT | PACKAGE));


I think we need kits to include the same build flags that packages do, except we should rebuild on a change to BUILDSYS_KITS_DIR vs BUILDSYS_PACKAGES_DIR.

I think we need kits to include the same build flags that packages do, except we should rebuild on a change to BUILDSYS_KITS_DIR vs BUILDSYS_PACKAGES_DIR.

True! I'll get it in the next one when adding the build-kit command and getting it to work.

bcressey · 2024-05-10T19:08:05Z

tools/buildsys/src/manifest.rs

+/// Lists include the "top-level manifest", i.e. the thing that `buildsys` is being asked to build.
+/// We do not want this, we want only a list of things that it depends on. Here we convert
+/// `PackageMetadata` objects to the `String` name, and filter out the "top-level manifest".
+fn filter_map_to_name(top_manifest_name: &str, pkg_metadata: &PackageMetadata) -> Option<String> {
+    if pkg_metadata.name() == top_manifest_name {
+        None
+    } else {
+        // Return the package override name, if it exists, or else the Cargo manifest name.
+        Some(get_buildsys_package_name(pkg_metadata))
+    }
+}


Are we guaranteed that the first entry in the traversal is the node we started from? Naively, that's what I would expect from a graph iterator. If so, we could just skip the first element from the iterator.

Are we guaranteed that the first entry in the traversal is the node we started from? Naively, that's what I would expect from a graph iterator. If so, we could just skip the first element from the iterator.

The documentation is silent on order of returned results. Seems safer and cheap to not make the assumption.

bcressey · 2024-05-10T19:14:25Z

tools/buildsys/src/manifest.rs

+        // Sort so that this function has consistent, dependable output regardless of graph internals.
+        packages.sort();
+        Ok(packages)


Does this make a difference? Seems like we'd already be getting topologically-sorted output, which should be reasonable consistent. This final sort ends up throwing away the ordering information.

Does this make a difference? Seems like we'd already be getting topologically-sorted output, which should be reasonable consistent. This final sort ends up throwing away the ordering information.

The documentation is silent on order of returned results (though topo-sort seems likely). My most common interaction with this list is "is the package I care about in the list?", whereby alphabetical is more convenient.

jmt-lab

LGTM

sam-berning · 2024-05-13T19:09:30Z

tools/buildsys/src/args.rs

    #[arg(long, env = "BUILDSYS_ARCH")]
    pub(crate) arch: SupportedArch,

+    #[arg(long, env = "BUILDSYS_CARGO_METADATA_PATH")]


does this need to be added to the rebuild vars?

does this need to be added to the rebuild vars?

I think we're covered by the fact that the package/variant dirs are being watched for changes, and the cargo metadata output should be the same unless something has changed about those packages. In other words, I think if anything important changes, we will be rebuilding anyway.

For kit builds, Buildsys will need to understand the cargo dependency graph. This requires a call to cargo metadata which, for efficiency sake, we do only once before calling cargo build. This commit updates Makefile.toml to provide that information to Buildsys. Additionally, we want to have one workspace at the top-level of the Twoliter project which includes all packages, kits and variants. This commit also includes changes to the Makefile.toml to support a single workspace.

webern · 2024-05-13T22:23:26Z

https://github.com/bottlerocket-os/twoliter/compare/5824391d09f6343f27c181f71d4ca98d6c30078a..654e237e8af43776e4fe07daa7b5ff8f65aab0a8

Fix a nit by moving a cargo clean in the Makefile to the clean-packages section.

bcressey reviewed Apr 30, 2024

View reviewed changes

webern mentioned this pull request Apr 30, 2024

design: build algorithm for kits #204

Closed

webern force-pushed the dep-walk branch from 0e06bb6 to c150a55 Compare May 1, 2024 18:43

webern force-pushed the dep-walk branch from c150a55 to 665c859 Compare May 2, 2024 00:37

bcressey reviewed May 2, 2024

View reviewed changes

tests/projects/local-kit/packages/pkg-d/Cargo.toml Outdated Show resolved Hide resolved

tools/buildsys/src/deps.rs Outdated Show resolved Hide resolved

tools/buildsys/src/deps.rs Outdated Show resolved Hide resolved

tools/buildsys/src/deps.rs Outdated Show resolved Hide resolved

webern force-pushed the dep-walk branch 4 times, most recently from c128614 to 596002d Compare May 3, 2024 23:46

webern requested a review from bcressey May 5, 2024 22:03

webern marked this pull request as ready for review May 5, 2024 22:04

webern mentioned this pull request May 7, 2024

release v0.2.0 #207

Closed

5 tasks

webern force-pushed the dep-walk branch from 596002d to e5ee4e1 Compare May 8, 2024 17:04

webern added 9 commits May 10, 2024 11:12

test-projects: remove unified-cgroup-hierarchy

ba2103d

test-project: add package to package dependency

fb4d0c0

test-project: use a single workspace

6dcf417

Twoliter projects, and kit builds in particular, will benefit from having a single workspace that includes all packages, kits and variants.

test-project: use a package name override

fca32a4

test-project: remove unused kit cargo.toml fields

6ef6d6c

buildsys: fix missing error messages in main.rs

8722ceb

The snafu errors in Buildsys/main.rs were printing the enum value only without any underlying error information,

buildsys: create a public build-type enum

9a99d41

buildsys: add a kit build-type

d06c5f0

buildsys: discover package and kit dependencies

5e437eb

webern force-pushed the dep-walk branch from e5ee4e1 to 5824391 Compare May 10, 2024 18:25

This was referenced May 10, 2024

update twoliter and buildsys to put RPMs into per-package sub-directories #209

Closed

update twoliter and buildsys to put RPMs into per-package sub-directories #210

Merged

bcressey approved these changes May 10, 2024

View reviewed changes

webern added the kits Work relating to kits. label May 13, 2024

jmt-lab approved these changes May 13, 2024

View reviewed changes

sam-berning approved these changes May 13, 2024

View reviewed changes

webern force-pushed the dep-walk branch from 5824391 to 654e237 Compare May 13, 2024 22:22

webern merged commit 958b544 into bottlerocket-os:develop May 13, 2024

webern deleted the dep-walk branch May 13, 2024 23:06

implement a dependency graph algorithm for package and kit builds #198

implement a dependency graph algorithm for package and kit builds #198

Uh oh!

Conversation

webern commented Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

webern commented May 1, 2024

Uh oh!

webern commented May 2, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

webern commented May 4, 2024

Uh oh!

webern commented May 5, 2024

Uh oh!

webern commented May 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmt-lab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webern commented May 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

webern commented Apr 13, 2024 •

edited

Loading