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

install: Compile (but error at runtime) on non-x86_64/aarch64 #114

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

jmarrero
Copy link
Member

We already are checking for target_arch when using the constants. Just instantiating them when we are in a target arch makes the build fail when we are building on a arch that that hits the constraint we created for initiating the constant.

closes #112

@openshift-ci
Copy link

openshift-ci bot commented Aug 15, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@jmarrero
Copy link
Member Author

jmarrero commented Aug 15, 2023

draft while I test this locally and a debug pod. This is just a theory atm... I can't see anything else that could be wrong.

@jmarrero jmarrero force-pushed the remove-unneaded-checks branch 4 times, most recently from 30d81a2 to 2808b77 Compare August 15, 2023 01:37
@jmarrero
Copy link
Member Author

It builds ok locally and I started up a s390x debug pod, ran:

cosa shell
sudo dnf install openssl-devel glib2-devel ostree-devel
cargo build

and got a good build:

   Compiling bootc v0.1.0 (/home/jenkins/agent/workspace/debug-pod/bootc/cli)
    Finished dev [optimized + debuginfo] target(s) in 2m 26s
[coreos-assembler]$ uname -a
Linux 1830b359aaec 6.3.12-200.fc38.s390x #1 SMP Thu Jul  6 03:39:39 UTC 2023 s390x GNU/Linux

@jmarrero jmarrero marked this pull request as ready for review August 15, 2023 14:19
@cgwalters
Copy link
Collaborator

A procedural note, we don't usually include the file suffix ".rs" in the topic prefix. This is about the install code (module). And it's usually best in the subject message to explain a bit more what the goal is. Something like:

install: Compile (but error at runtime) on non-x86_64/aarch64

This all said, while I'm not opposed to this fix...thinking about this a bit more, what seems it'd be better (and about equally easy) is to change the build configuration in Cargo.toml to only enable the install feature on x86_64 and ppc64le.

Then we can drop many of the architecture conditionals in that code. Conceptually we'd be going from bootc install on those arches saying "This isn't implemented" to "command not found" which I think is probably more right.

Hmm...I'm not totally sure Cargo has support for conditionally enabling a crate feature for a given arch (without resorting to a build script). It does support this for dependencies https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies

Well...in the end, let's just do some cleanup for this commit message and merge!

(Of course the real fix is to actually implement the install flow for those arches, but that's more involved)

@cgwalters
Copy link
Collaborator

BTW just for reference this install logic is a Rust rewrite of https://github.com/coreos/coreos-assembler/blob/main/src/create_disk.sh

We already are checking for target_arch when using the constants.
Just instantiating them when we are in a target_arch makes the build
fail when we are building on non-x86_64/aarch64.

Signed-off-by: Joseph Marrero <[email protected]>
@jmarrero jmarrero force-pushed the remove-unneaded-checks branch from 2808b77 to 5f42643 Compare August 15, 2023 19:03
@jmarrero
Copy link
Member Author

Thanks, yeah my main intent here is just to not break rpm-ostree builds by vendoring bootc for now. I found https://github.com/coreos/coreos-assembler/blob/main/src/create_disk.sh when looking for EFIPN which I am still not sure what it is 🥲

@cgwalters cgwalters changed the title baseline.rs: remove constant declaration checks per arch install: Compile (but error at runtime) on non-x86_64/aarch64 Aug 15, 2023
@cgwalters cgwalters merged commit 8ecd2bf into containers:main Aug 15, 2023
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.

Build fails on s390x:
2 participants