-
Notifications
You must be signed in to change notification settings - Fork 39
buildsys: use clap for env vars #134
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,214 @@ | ||
| /*! | ||
|
|
||
| These structs provide the CLI interface for buildsys which is called from Cargo.toml and accepts all | ||
| of its input arguments from environment variables. | ||
|
|
||
| !*/ | ||
|
|
||
| use buildsys::manifest::SupportedArch; | ||
| use clap::{Parser, Subcommand}; | ||
| use std::path::PathBuf; | ||
| use url::Url; | ||
|
|
||
| /// 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), | ||
| ("BUILDSYS_NAME", VARIANT), | ||
| ("BUILDSYS_OUTPUT_DIR", VARIANT), | ||
| ("BUILDSYS_PACKAGES_DIR", PACKAGE), | ||
|
Comment on lines
+20
to
+21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it feels like a (pre-existing) mistake to not track
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
| ("BUILDSYS_PRETTY_NAME", VARIANT), | ||
| ("BUILDSYS_ROOT_DIR", PACKAGE | VARIANT), | ||
| ("BUILDSYS_STATE_DIR", PACKAGE | VARIANT), | ||
| ("BUILDSYS_TIMESTAMP", VARIANT), | ||
| ("BUILDSYS_VARIANT", VARIANT), | ||
| ("BUILDSYS_VERSION_BUILD", VARIANT), | ||
| ("BUILDSYS_VERSION_IMAGE", VARIANT), | ||
| ("TLPRIVATE_SDK_IMAGE", PACKAGE | VARIANT), | ||
| ("TLPRIVATE_TOOLCHAIN", PACKAGE | VARIANT), | ||
| ]; | ||
|
|
||
| /// A tool for building Bottlerocket images and artifacts. | ||
| #[derive(Debug, Parser)] | ||
| pub(crate) struct Buildsys { | ||
| #[command(subcommand)] | ||
| pub(crate) command: Command, | ||
| } | ||
|
|
||
| #[derive(Subcommand, Debug)] | ||
| pub(crate) enum Command { | ||
| BuildPackage(Box<BuildPackageArgs>), | ||
| BuildVariant(Box<BuildVariantArgs>), | ||
| } | ||
|
|
||
| impl Command { | ||
| pub(crate) fn build_type(&self) -> BuildType { | ||
| match self { | ||
| Command::BuildPackage(_) => BuildType::Package, | ||
| Command::BuildVariant(_) => BuildType::Variant, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Arguments common to all subcommands. | ||
| #[derive(Debug, Parser)] | ||
| pub(crate) struct Common { | ||
| #[arg(long, env = "BUILDSYS_ARCH")] | ||
| pub(crate) arch: SupportedArch, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_OUTPUT_DIR")] | ||
| pub(crate) image_arch_variant_dir: PathBuf, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_ROOT_DIR")] | ||
| pub(crate) root_dir: PathBuf, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_STATE_DIR")] | ||
| pub(crate) state_dir: PathBuf, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_TIMESTAMP")] | ||
| pub(crate) timestamp: String, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_VERSION_FULL")] | ||
| pub(crate) version_full: String, | ||
|
|
||
| #[arg(long, env = "CARGO_MANIFEST_DIR")] | ||
| pub(crate) cargo_manifest_dir: PathBuf, | ||
|
Comment on lines
+76
to
+77
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My preference would be to include it here anyway for two reasons:
|
||
|
|
||
| #[arg(long, env = "TLPRIVATE_SDK_IMAGE")] | ||
| pub(crate) sdk_image: String, | ||
|
|
||
| #[arg(long, env = "TLPRIVATE_TOOLCHAIN")] | ||
| pub(crate) toolchain: String, | ||
|
|
||
| #[arg(long, env = "TWOLITER_TOOLS_DIR")] | ||
| pub(crate) tools_dir: PathBuf, | ||
| } | ||
|
|
||
| /// Build RPMs from a spec file and sources. | ||
| #[derive(Debug, Parser)] | ||
| pub(crate) struct BuildPackageArgs { | ||
| #[arg(long, env = "BUILDSYS_PACKAGES_DIR")] | ||
| pub(crate) packages_dir: PathBuf, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_VARIANT")] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This goes away once we no longer have conditional compilation right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That should be the goal for packages, yes. |
||
| pub(crate) variant: String, | ||
|
Comment on lines
+95
to
+96
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To check my understanding - we don't want this in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's the idea, yeah. And I think our goal should be that package builds also don't need to know the variant name. |
||
|
|
||
| #[arg(long, env = "BUILDSYS_VARIANT_PLATFORM")] | ||
| pub(crate) variant_platform: String, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_VARIANT_RUNTIME")] | ||
| pub(crate) variant_runtime: String, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_VARIANT_FAMILY")] | ||
| pub(crate) variant_family: String, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_VARIANT_FLAVOR")] | ||
| pub(crate) variant_flavor: String, | ||
|
|
||
| #[arg(long, env = "PUBLISH_REPO")] | ||
| pub(crate) publish_repo: String, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_SOURCES_DIR")] | ||
| pub(crate) sources_dir: PathBuf, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_LOOKASIDE_CACHE")] | ||
| pub(crate) lookaside_cache: Url, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_UPSTREAM_SOURCE_FALLBACK")] | ||
| pub(crate) upstream_source_fallback: String, | ||
|
|
||
| #[arg(long, env = "CARGO_PKG_NAME")] | ||
| pub(crate) cargo_package_name: String, | ||
|
|
||
| #[command(flatten)] | ||
| pub(crate) common: Common, | ||
| } | ||
|
|
||
| /// Build filesystem and disk images from RPMs. | ||
| #[derive(Debug, Parser)] | ||
| pub(crate) struct BuildVariantArgs { | ||
| #[arg(long, env = "BUILDSYS_NAME")] | ||
| pub(crate) name: String, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_PRETTY_NAME")] | ||
| pub(crate) pretty_name: String, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_VARIANT")] | ||
| pub(crate) variant: String, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_VERSION_BUILD")] | ||
| pub(crate) version_build: String, | ||
|
|
||
| #[arg(long, env = "BUILDSYS_VERSION_IMAGE")] | ||
| pub(crate) version_image: String, | ||
|
|
||
| #[command(flatten)] | ||
| pub(crate) common: Common, | ||
| } | ||
|
|
||
| /// Returns the environment variables that need to be watched for a given `[BuildType]`. | ||
| fn sensitive_env_vars(build_type: BuildType) -> impl Iterator<Item = &'static str> { | ||
| REBUILD_VARS | ||
| .into_iter() | ||
| .filter(move |(_, flags)| build_type.includes(*flags)) | ||
| .map(|(var, _)| var) | ||
| } | ||
|
|
||
| /// Emits the cargo directives for a the list of sensitive environment variables for a given | ||
| /// `[BuildType]`. | ||
| pub(crate) fn rerun_for_envs(build_type: BuildType) { | ||
| for var in sensitive_env_vars(build_type) { | ||
| println!("cargo:rerun-if-env-changed={}", var) | ||
| } | ||
| } | ||
|
|
||
| /// The thing that buildsys is building. | ||
| #[repr(u8)] | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub(crate) enum BuildType { | ||
| Package = 0b00000001, | ||
| Variant = 0b00000010, | ||
| } | ||
|
|
||
| impl BuildType { | ||
| fn includes(&self, flags: u8) -> bool { | ||
| let this = *self as u8; | ||
| let and = flags & this; | ||
| and == this | ||
| } | ||
| } | ||
|
|
||
| const PACKAGE: u8 = BuildType::Package as u8; | ||
| const VARIANT: u8 = BuildType::Variant as u8; | ||
|
|
||
| #[test] | ||
| fn build_type_includes_test() { | ||
| // true | ||
| assert!(BuildType::Package.includes(PACKAGE | VARIANT)); | ||
| assert!(BuildType::Variant.includes(VARIANT)); | ||
| assert!(BuildType::Variant.includes(VARIANT | PACKAGE)); | ||
|
|
||
| // false | ||
| assert!(!BuildType::Package.includes(VARIANT)); | ||
| assert!(!BuildType::Variant.includes(PACKAGE)); | ||
| assert!(!BuildType::Variant.includes(32)); | ||
| assert!(!BuildType::Variant.includes(0)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_sensitive_env_vars_variant() { | ||
| let list: Vec<&str> = sensitive_env_vars(BuildType::Variant).collect(); | ||
| assert!(list.contains(&"BUILDSYS_ARCH")); | ||
| assert!(list.contains(&"BUILDSYS_VARIANT")); | ||
| assert!(!list.contains(&"BUILDSYS_PACKAGES_DIR")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_sensitive_env_vars_package() { | ||
| let list: Vec<&str> = sensitive_env_vars(BuildType::Package).collect(); | ||
| assert!(list.contains(&"BUILDSYS_ARCH")); | ||
| assert!(list.contains(&"BUILDSYS_PACKAGES_DIR")); | ||
| assert!(!list.contains(&"BUILDSYS_VARIANT")); | ||
| } | ||
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 is a nice way to organize something that was very ad-hoc and organic before.