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

CI: Set -Crelocation-model=static in RUSTFLAGS for bootloader test job #480

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

phil-opp
Copy link
Member

@phil-opp phil-opp commented Apr 8, 2024

In d3b9c05, we switched our test framework to the built-in x86_64-unknown-none target. This required a -Crelocation-model=static flag to work with bootloader v0.9, which we set using a build.rustflags config key.

In 2eb838d, we set a RUSTFLAGS env variable on CI to deny warnings.

Unfortunately, the above two commits conflict because the RUSTFLAGS env variable takes precedence over the build.rustflags key. This commit fixes this by setting an explicit RUSTFLAGS value for the bootloader test job that contains both flags.

This should fix the CI build errors.

In d3b9c05, we switched our test framework to the built-in x86_64-unknown-none target. This required a `-Crelocation-model=static` flag to work with bootloader v0.9, which we set using a `build.rustflags` config key.

In 2eb838d, we set a `RUSTFLAGS` env variable on CI to deny warnings.

Unfortunately, the above two commits conflict because the `RUSTFLAGS` env variable takes precedence over the `build.rustflags` key. This commit fixes this by setting an explicit `RUSTFLAGS` value for the bootloader test job that contains both flags.
@phil-opp phil-opp marked this pull request as ready for review April 8, 2024 08:55
Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

Merge queues would have prevented this. Maybe we should consider enabling them?

@phil-opp
Copy link
Member Author

phil-opp commented Apr 8, 2024

Enabling merge queues is a good idea! However, the "Bootloader Integration Test" job is not marked as required, so merge queues wouldn't have helped here.

The reason for keeping the bootloader integration test jobs optional is to avoid deadlock situations on nightly breakage. There is a circular dependency because the bootloader crate depends on x86_64, which uses bootloader again for testing. So in case of nightly breakage we need to be able to fix x86_64 first.

@mkroening
Copy link
Member

Ah, that makes sense. :)

Copy link
Member

@Freax13 Freax13 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

.github/workflows/build.yml Show resolved Hide resolved
@phil-opp phil-opp merged commit 615248f into master Apr 8, 2024
12 checks passed
@phil-opp phil-opp deleted the fix-bootloader-test branch April 8, 2024 11:11
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.

3 participants