Skip to content
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

add sdk to project schema #89

Merged
merged 2 commits into from
Oct 3, 2023
Merged

Conversation

webern
Copy link
Contributor

@webern webern commented Sep 29, 2023

Issue #, if available:

Closes #83

Description of changes:

Add the ability to specify the desired Bottlerocket SDK in
Twoliter.toml.
Require the SDK to be specified in Twoliter.toml when using twoliter
make.

Testing

I used this version of Twoliter without changing anything in Bottlerocket and got the expected error:

Error: The environment variable 'BUILDSYS_SDK_NAME' can no longer be used. Specify the SDK in Twoliter.toml
[cargo-make] ERROR - Error while executing command, exit code: 1

I then removed these vars from Bottlerocket's Makefile.toml and got the expected error:

Error: When using twoliter make, it is required that the SDK be specified in Twoliter.toml

I added the SDK to Bottlerocket's Twoliter.toml and the build succeeded and the correct SDK was used:

v0.34.1: Pulling from bottlerocket/bottlerocket-sdk-x86_64

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@webern webern changed the title Sdk schema add sdk to project schema Sep 29, 2023
@webern webern force-pushed the sdk-schema branch 2 times, most recently from d71e833 to 9dff50a Compare September 29, 2023 21:13
@webern webern marked this pull request as ready for review September 29, 2023 21:51
@webern webern requested review from bcressey and ecpullen September 29, 2023 21:51
Comment on lines +116 to +128
/// A base name for an image that can be suffixed using a naming convention. For example,
/// `registry=public.ecr.aws/bottlerocket`, `name=bottlerocket`, `version=v0.50.0` can be suffixed
/// via naming convention to produce:
/// - `registry=public.ecr.aws/bottlerocket/bottlerocket-sdk-x86_64:v0.50.0`
/// - `registry=public.ecr.aws/bottlerocket/bottlerocket-toolchain-aarch64:v0.50.0`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get rid of the "magic" implicit suffixing if we can since ultimately that constrains the end user in kind of an arbitrary way.

What do you think about making the name a template like bottlerocket-sdk-{{ arch }}? That's consistent with similar patterns elsewhere in the project.

I would also recommend making the toolchain image its own explicit field, e.g. toolchain_name . But toolchain (and kmod kits) are just inherently problematic so we'll need to find a different way to deal with them at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this and didn't like the suffixing when I encountered it. I'm fine with changing to the non-magical version. It's not great to have a field in the schema that needs to be removed at some point. Any idea of a way to avoid having a schema key that is effectively temporary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like the correct fix is to have the SDK always build a "native" toolchain, possibly in addition to the "cross" toolchain, so that the SDK itself always has the archive and we don't need to keep a second container in sync. We do something like this for Rust already, for different reasons.

I've opened bottlerocket-os/bottlerocket-sdk#137 for that.

Any idea of a way to avoid having a schema key that is effectively temporary?

Not really. We could have a fallback path, like "if the toolchain key isn't set, assume the SDK has the toolchain and use it instead".

@webern
Copy link
Contributor Author

webern commented Oct 2, 2023

webern added 2 commits October 2, 2023 15:02
Add the ability to specify the desired Bottlerocket SDK in
Twoliter.toml.
Require the SDK to be specified in Twoliter.toml when using twoliter
make.
@webern
Copy link
Contributor Author

webern commented Oct 2, 2023

@webern webern merged commit bca8e8b into bottlerocket-os:develop Oct 3, 2023
1 check passed
@webern webern deleted the sdk-schema branch October 3, 2023 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

alpha: take sdk version from twoliter.toml
3 participants