buildsys: use clap for env vars #134

webern · 2024-01-17T20:55:12Z

Issue number:

Related to, refactor before #70

Description of changes:

Required input in the form of environment variables was sprinkled throughout the code. Here we aggregate the inputs into a CLI interface using Clap, while still allowing all to specified via environment variables.

Testing done:

Built an aws-k8s-1.27 image
Built it again and observed that packages were not rebuilt
touched a file (binutils.spec) and built it again - observed that binutils was rebuilt but other packages were not
Built an aws-ecs-2 image, many (but not all) packages were rebuilt due to changed variant
Built it again and observed that packages were not rebuilt
touched a file (binutils.spec) and built it again - observed that binutils was rebuilt but other packages were not

Also, during the refactoring process, I captured the output of the docker build command before and after this PR's changes. I found that I was getting the same build args and secrets args before and after (with the exception that SDK, TOOLCHAIN, TOKEN and NOCACHE have been added during build-variant, which seems fine).

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.

bcressey · 2024-01-18T20:41:08Z

tools/buildsys/src/builder.rs

+            &build_variant_args.common.output_dir,
+            &build_variant_args.common.root_dir,
+            &build_variant_args.common.state_dir,
+            &build_variant_args.common.sdk_image,
+            &build_variant_args.common.toolchain,


I see there's a TODO for this, but it would be nice to have a BuildContext type to hold these values so that the build function signature isn't so complicated.

bcressey · 2024-01-18T20:44:07Z

tools/buildsys/src/builder.rs

+        let arch = &build_variant_args.common.arch;
+        let goarch = serde_plain::from_str::<SupportedArch>(arch)
+            .context(error::UnsupportedArchSnafu { arch })?


Can we do this conversion from &str to SupportedArch just once, perhaps when parsing arguments? Otherwise we end up doing it in a few different places - here and every time supported_arch is called.

bcressey · 2024-01-18T20:47:41Z

tools/buildsys/src/cache.rs

@@ -79,11 +78,9 @@ impl LookasideCache {

    /// Retrieves a file from the specified URL and write it to the given path,
    /// then verifies the contents against the SHA-512 hash provided.
-    fn fetch_file<P: AsRef<Path>>(url: &str, path: P, hash: &str) -> Result<()> {
+    fn fetch_file<P: AsRef<Path>>(url: &str, path: P, hash: &str, version: &str) -> Result<()> {


I don't love this function signature change - it feels like one thing to add additional context to a request header based on an environment variable, and another to take an otherwise useless parameter just to do the same.

I'd prefer to either not emit the version at all - do we really care? - or else to add it as a field to LookasideCache, pass it into LookasideCache::new, and retrieve it from self.

I don't know if we care about emitting the version, so I've kept it but with your suggestion to make it self.version.

tools/buildsys/src/args.rs

bcressey · 2024-01-18T22:04:03Z

tools/buildsys/src/args.rs

+            "BUILDSYS_PACKAGES_DIR",
+            "BUILDSYS_SOURCES_DIR",


I'm OK with unconditionally emitting these for variables we expect to be constant for the life of a given workspace, if we want to simplify slightly and confine the package- and variant-specific rerun_for_envs() to truly special cases.

tools/buildsys/src/args.rs

bcressey · 2024-01-18T22:13:14Z

tools/buildsys/src/args.rs

+    #[arg(long, env = "CARGO_MANIFEST_DIR")]
+    pub(crate) cargo_manifest_dir: PathBuf,


I'm not as excited about converting cargo's own environment variables into arguments since the environment is the canonical way to pass this information.

My preference would be to include it here anyway for two reasons:

Readability: self-documenting code

If I want to write buildsys tests, I may call it directly instead of invoking it from a build.rs script and using args might be helpful for that

webern · 2024-01-20T02:30:42Z

https://github.com/bottlerocket-os/twoliter/compare/b535980ed80a54ce9aaa803de228e4120a3ad096..a1eae7b2165900eca3e07e754c7c0b5a09469b89

Fixup the cargo:rerun-if-env-changed list to make it the same as it used to be.

webern · 2024-01-21T20:25:14Z

https://github.com/bottlerocket-os/twoliter/compare/a1eae7b2165900eca3e07e754c7c0b5a09469b89..c71f56ece7a6e1df398e5552a96c756b34a1e51f

Make the version a parameter of LookasideCache

webern · 2024-01-23T18:28:25Z

I've gotten myself into a much bigger refactor of builder.rs so converted this to a draft.

webern · 2024-01-25T00:55:56Z

@bcressey this is ready for you to re-review. The refactor is rather substantial, but I think it is an improvement. My goal was to separate the concerns of a) gathering data from the environment, b) manufacturing and constructing args for docker, and c) the program flow of calling docker. Previously all of those things were happening in the same code-paths. I think having them in separate code paths will make it easier to hack on this for kits.

webern · 2024-02-02T01:04:29Z

https://github.com/bottlerocket-os/twoliter/compare/9a89bf58869592d3f451af59a5cb35b5c81a9329..be37869a03eda322ed07e527aacd978a5b0a8bb9

Slightly painful rebase.

Required input in the form of environment variables was sprinkled throughout the code. Here we aggregate the inputs into a CLI interface using Clap, while still allowing all to specified via environment variables.

webern · 2024-02-02T01:11:50Z

https://github.com/bottlerocket-os/twoliter/compare/be37869a03eda322ed07e527aacd978a5b0a8bb9..8b857d98fb2aed710b662049273591b2e2888596

Clippy fix.

bcressey · 2024-02-08T23:15:52Z

tools/buildsys/src/builder.rs

+                    .image_features()
+                    .map(|set| set.iter().map(|&x| x.to_owned()).collect()),
+                package,
+                publish_repo: args.publish_repo,


I opened #151 to correct this since it felt out of place here.

bcressey · 2024-02-08T23:23:47Z

tools/buildsys/src/builder.rs

+            "build {context} \
        --target {target} \
        --tag {tag} \
        --file {dockerfile}",
-        dockerfile = dockerfile.display(),
-        target = target,
-        tag = tag,
-    )
-    .split_string();
-
-    build.extend(build_args);
-    build.build_arg("SDK", sdk);
-    build.build_arg("TOOLCHAIN", toolchain);
-    build.build_arg("NOCACHE", nocache.to_string());
-    // Avoid using a cached layer from a concurrent build in another checkout.
-    build.build_arg("TOKEN", token);
-
-    let create = format!("create --name {} {} true", tag, tag).split_string();
-    let cp = format!("cp {}:/output/. {}", tag, build_dir.display()).split_string();
-    let rm = format!("rm --force {}", tag).split_string();
-    let rmi = format!("rmi --force {}", tag).split_string();
-
-    // Clean up the stopped container if it exists.
-    let _ = docker(&rm, Retry::No);
-
-    // Clean up the previous image if it exists.
-    let _ = docker(&rmi, Retry::No);
-
-    // Build the image, which builds the artifacts we want.
-    // Work around transient, known failure cases with Docker.
-    docker(
-        &build,
-        Retry::Yes {
-            attempts: DOCKER_BUILD_MAX_ATTEMPTS,
-            messages: &[
-                &*DOCKER_BUILD_FRONTEND_ERROR,
-                &*DOCKER_BUILD_DEAD_RECORD_ERROR,
-                &*UNEXPECTED_EOF_ERROR,
-                &*CREATEREPO_C_READ_HEADER_ERROR,
-            ],
-        },
-    )?;
-
-    // Create a stopped container so we can copy artifacts out.
-    docker(&create, Retry::No)?;
-
-    // Copy artifacts into our output directory.
-    docker(&cp, Retry::No)?;
-
-    // Clean up our stopped container after copying artifacts out.
-    docker(&rm, Retry::No)?;
-
-    // Clean up our image now that we're done.
-    docker(&rmi, Retry::No)?;
-
-    // Copy artifacts to the expected directory and write markers to track them.
-    copy_build_files(&build_dir, output_dir)?;
+            context = self.context.display(),
+            dockerfile = self.dockerfile.display(),
+            target = self.target,
+            tag = self.tag,


nit: the spacing on these lines seems off?

nit: the spacing on these lines seems off?

Yeah fmt is no help here. I've now aligned the multiline string line starts with the variable list below it.

bcressey · 2024-02-08T23:34:54Z

tools/buildsys/src/args.rs

+/// A list of environment variables and the type of build that should be rerun if that environment
+/// variable changes. The build type is represented with bit flags so that we can easily list
+/// multiple build types for a single variable. See `[BuildType]` and `[rerun_for_envs]` below to
+/// see how this list is used.
+const REBUILD_VARS: [(&str, u8); 13] = [
+    ("BUILDSYS_ARCH", PACKAGE | VARIANT),


❤️

This is a nice way to organize something that was very ad-hoc and organic before.

bcressey · 2024-02-08T23:42:20Z

tools/buildsys/src/args.rs

+    ("BUILDSYS_OUTPUT_DIR", VARIANT),
+    ("BUILDSYS_PACKAGES_DIR", PACKAGE),


nit: it feels like a (pre-existing) mistake to not track BUILDSYS_SOURCES_DIR if we are tracking BUILDSYS_PACKAGES_DIR - if we override the build to point to a completely different sources tree then that should invalidate any builds that used those sources.

nit: it feels like a (pre-existing) mistake to not track BUILDSYS_SOURCES_DIR if we are tracking BUILDSYS_PACKAGES_DIR - if we override the build to point to a completely different sources tree then that should invalidate any builds that used those sources.

I'm going to defer this because I'm concerned that the symlink change when changing variant could trigger something. I'll track this as a GitHub issue and play around with adding it. #152

Edit: no that's dumb. I can safely add this.

Edit, edit: Actually I'm still squeamish... deferring.

bcressey · 2024-02-08T23:47:42Z

tools/buildsys/src/args.rs

+    #[arg(long, env = "BUILDSYS_VARIANT")]
+    pub(crate) variant: String,


To check my understanding - we don't want this in Common because kits won't need this variable?

To check my understanding - we don't want this in Common because kits won't need this variable?

That's the idea, yeah. And I think our goal should be that package builds also don't need to know the variant name.

ecpullen

LGTM

ecpullen · 2024-02-09T00:10:13Z

tools/buildsys/src/args.rs

+    #[arg(long, env = "BUILDSYS_PACKAGES_DIR")]
+    pub(crate) packages_dir: PathBuf,
+
+    #[arg(long, env = "BUILDSYS_VARIANT")]


This goes away once we no longer have conditional compilation right?

This goes away once we no longer have conditional compilation right?

That should be the goal for packages, yes.

yeazelm · 2024-02-09T22:38:21Z

tools/buildsys/src/builder.rs

-    let mut build = format!(
-        "build . \
+        let mut build = format!(
+            "build {context} \


nit: this intending looks strange to me?

nit: this intending looks strange to me?

Yeah fmt is no help here. I've now aligned the multiline string line starts with the variable list below it.

yeazelm · 2024-02-09T22:44:12Z

tools/buildsys/src/builder.rs

@@ -358,6 +443,8 @@ fn docker(args: &[String], retry: Retry) -> Result<Output> {
        retry_messages = messages;
    }

+    println!("Running command:\ndocker {}", args.join(" /\n"));


This is only one place out of two we println!() in this file. Is that intentional? I think so but wanted to ensure this isn't a hold over from debugging and this output is doing something useful for logs/output for the user?

This is only one place out of two we println!() in this file. Is that intentional? I think so but wanted to ensure this isn't a hold over from debugging and this output is doing something useful for logs/output for the user?

Good catch. Fixed.

webern · 2024-01-25T00:58:43Z

tools/buildsys/src/builder.rs

+    target: String,
+    tag: String,
+    root_dir: PathBuf,
+    // build_dir: PathBuf,


I fixed this.

Suggested change

// build_dir: PathBuf,

webern · 2024-02-10T01:32:54Z

tools/buildsys/src/builder.rs

@@ -358,6 +443,8 @@ fn docker(args: &[String], retry: Retry) -> Result<Output> {
        retry_messages = messages;
    }

+    println!("Running command:\ndocker {}", args.join(" /\n"));


This is only one place out of two we println!() in this file. Is that intentional? I think so but wanted to ensure this isn't a hold over from debugging and this output is doing something useful for logs/output for the user?

Good catch. Fixed.

webern · 2024-02-10T01:35:34Z

tools/buildsys/src/builder.rs

+            "build {context} \
        --target {target} \
        --tag {tag} \
        --file {dockerfile}",
-        dockerfile = dockerfile.display(),
-        target = target,
-        tag = tag,
-    )
-    .split_string();
-
-    build.extend(build_args);
-    build.build_arg("SDK", sdk);
-    build.build_arg("TOOLCHAIN", toolchain);
-    build.build_arg("NOCACHE", nocache.to_string());
-    // Avoid using a cached layer from a concurrent build in another checkout.
-    build.build_arg("TOKEN", token);
-
-    let create = format!("create --name {} {} true", tag, tag).split_string();
-    let cp = format!("cp {}:/output/. {}", tag, build_dir.display()).split_string();
-    let rm = format!("rm --force {}", tag).split_string();
-    let rmi = format!("rmi --force {}", tag).split_string();
-
-    // Clean up the stopped container if it exists.
-    let _ = docker(&rm, Retry::No);
-
-    // Clean up the previous image if it exists.
-    let _ = docker(&rmi, Retry::No);
-
-    // Build the image, which builds the artifacts we want.
-    // Work around transient, known failure cases with Docker.
-    docker(
-        &build,
-        Retry::Yes {
-            attempts: DOCKER_BUILD_MAX_ATTEMPTS,
-            messages: &[
-                &*DOCKER_BUILD_FRONTEND_ERROR,
-                &*DOCKER_BUILD_DEAD_RECORD_ERROR,
-                &*UNEXPECTED_EOF_ERROR,
-                &*CREATEREPO_C_READ_HEADER_ERROR,
-            ],
-        },
-    )?;
-
-    // Create a stopped container so we can copy artifacts out.
-    docker(&create, Retry::No)?;
-
-    // Copy artifacts into our output directory.
-    docker(&cp, Retry::No)?;
-
-    // Clean up our stopped container after copying artifacts out.
-    docker(&rm, Retry::No)?;
-
-    // Clean up our image now that we're done.
-    docker(&rmi, Retry::No)?;
-
-    // Copy artifacts to the expected directory and write markers to track them.
-    copy_build_files(&build_dir, output_dir)?;
+            context = self.context.display(),
+            dockerfile = self.dockerfile.display(),
+            target = self.target,
+            tag = self.tag,


nit: the spacing on these lines seems off?

Yeah fmt is no help here. I've now aligned the multiline string line starts with the variable list below it.

webern · 2024-02-10T01:35:42Z

tools/buildsys/src/builder.rs

-    let mut build = format!(
-        "build . \
+        let mut build = format!(
+            "build {context} \


nit: this intending looks strange to me?

Yeah fmt is no help here. I've now aligned the multiline string line starts with the variable list below it.

webern · 2024-02-10T01:38:06Z

tools/buildsys/src/args.rs

+    #[arg(long, env = "BUILDSYS_PACKAGES_DIR")]
+    pub(crate) packages_dir: PathBuf,
+
+    #[arg(long, env = "BUILDSYS_VARIANT")]


This goes away once we no longer have conditional compilation right?

That should be the goal for packages, yes.

webern · 2024-02-10T01:40:06Z

tools/buildsys/src/args.rs

+    #[arg(long, env = "BUILDSYS_VARIANT")]
+    pub(crate) variant: String,


To check my understanding - we don't want this in Common because kits won't need this variable?

That's the idea, yeah. And I think our goal should be that package builds also don't need to know the variant name.

webern · 2024-02-10T01:41:26Z

tools/buildsys/src/args.rs

+    ("BUILDSYS_OUTPUT_DIR", VARIANT),
+    ("BUILDSYS_PACKAGES_DIR", PACKAGE),


nit: it feels like a (pre-existing) mistake to not track BUILDSYS_SOURCES_DIR if we are tracking BUILDSYS_PACKAGES_DIR - if we override the build to point to a completely different sources tree then that should invalidate any builds that used those sources.

I'm going to defer this because I'm concerned that the symlink change when changing variant could trigger something. I'll track this as a GitHub issue and play around with adding it. #152

Edit: no that's dumb. I can safely add this.

Edit, edit: Actually I'm still squeamish... deferring.

Refactor the code, primarily in builder.rs. The goal of the refactor is to segregate data collection and docker argument creation from program flow logic.

webern force-pushed the buildsys-args branch 6 times, most recently from 5086222 to b535980 Compare January 18, 2024 01:10

webern marked this pull request as ready for review January 18, 2024 19:23

bcressey reviewed Jan 18, 2024

View reviewed changes

webern force-pushed the buildsys-args branch from b535980 to a1eae7b Compare January 20, 2024 02:11

webern force-pushed the buildsys-args branch from a1eae7b to c71f56e Compare January 21, 2024 20:23

webern marked this pull request as draft January 23, 2024 18:27

webern marked this pull request as ready for review January 25, 2024 00:56

webern requested a review from bcressey January 25, 2024 00:56

webern force-pushed the buildsys-args branch from b5645cd to 3203989 Compare January 25, 2024 01:23

webern changed the base branch from develop to toolsdir January 25, 2024 01:26

webern force-pushed the buildsys-args branch from 3203989 to 9a89bf5 Compare January 25, 2024 01:27

webern force-pushed the toolsdir branch 2 times, most recently from c822ce8 to 7f95550 Compare January 29, 2024 19:57

Base automatically changed from toolsdir to develop January 29, 2024 23:50

webern force-pushed the buildsys-args branch from 9a89bf5 to be37869 Compare February 2, 2024 01:04

buildsys: use clap for env vars

52cf9ba

Required input in the form of environment variables was sprinkled throughout the code. Here we aggregate the inputs into a CLI interface using Clap, while still allowing all to specified via environment variables.

webern force-pushed the buildsys-args branch from be37869 to 8b857d9 Compare February 2, 2024 01:11

bcressey approved these changes Feb 8, 2024

View reviewed changes

ecpullen approved these changes Feb 9, 2024

View reviewed changes

yeazelm approved these changes Feb 9, 2024

View reviewed changes

webern mentioned this pull request Feb 10, 2024

Track BUILDSYS_SOURCES_DIR for rebuilds in buildsys #152

Closed

webern commented Feb 10, 2024

View reviewed changes

buildsys: refactor builder.rs

92bc3bb

Refactor the code, primarily in builder.rs. The goal of the refactor is to segregate data collection and docker argument creation from program flow logic.

webern force-pushed the buildsys-args branch from 8b857d9 to 92bc3bb Compare February 10, 2024 01:48

webern merged commit 6fec72b into develop Feb 12, 2024
1 check passed

webern deleted the buildsys-args branch February 12, 2024 18:08

		#[arg(long, env = "CARGO_MANIFEST_DIR")]
		pub(crate) cargo_manifest_dir: PathBuf,

		("BUILDSYS_OUTPUT_DIR", VARIANT),
		("BUILDSYS_PACKAGES_DIR", PACKAGE),

		#[arg(long, env = "BUILDSYS_VARIANT")]
		pub(crate) variant: String,

buildsys: use clap for env vars #134

buildsys: use clap for env vars #134

Conversation

webern commented Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webern commented Jan 20, 2024

webern commented Jan 21, 2024

webern commented Jan 23, 2024

webern commented Jan 25, 2024

webern commented Feb 2, 2024

webern commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webern Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecpullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webern Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

webern commented Jan 17, 2024 •

edited

Loading

webern Feb 10, 2024 •

edited

Loading

webern Feb 10, 2024 •

edited

Loading