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

riscv: build: make cfg variables more robust #205

Merged
merged 4 commits into from
May 7, 2024

Conversation

rmsyn
Copy link
Contributor

@rmsyn rmsyn commented May 4, 2024

Change which Cargo CFG environment variables are used to select the custom cfg variables.

See #204 (comment) for discussion.

riscv/build.rs Outdated Show resolved Hide resolved
riscv/CHANGELOG.md Outdated Show resolved Hide resolved
Change which Cargo `CFG` environment variables are used to select the
custom `cfg` variables.

See
rust-embedded#204 (comment)
for discussion.
@rmsyn
Copy link
Contributor Author

rmsyn commented May 5, 2024

I don't know why the clippy checks are suddenly failing now. I run the checks locally from CI runner:

cargo clippy --package riscv-rt --all --features=s-mode -- -D warnings

The checks pass. 🤔

@romancardenas
Copy link
Contributor

romancardenas commented May 5, 2024

In nightly, clippy now complains about "custom" compiler features (check this).

So, as suggested by the CI, now we should add to build.rs something like:

println!("cargo::rustc-check-cfg=cfg(riscv)");
println!("cargo::rustc-check-cfg=cfg(riscv32)");
println!("cargo::rustc-check-cfg=cfg(riscv64)");

Copy link
Contributor

@romancardenas romancardenas 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! However, we still need to address new clippy requirements

@rmsyn
Copy link
Contributor Author

rmsyn commented May 5, 2024

Build checks on 1.60 machines are failing because of bumped MSRV for the cfg checks.

Without an MSRV bump, the clippy checks have the following error:

cargo +nightly clippy --all --no-default-features --target riscv64imac-unknown-none-elf -- -D warnings
   Compiling riscv v0.11.1 (/riscv/riscv)
   Compiling riscv-rt v0.13.0 (/riscv/riscv-rt)
error: the `cargo::` syntax for build script output instructions was added in Rust 1.77.0, but the minimum supported Rust version of `riscv v0.11.1 (/riscv/riscv)` is 1.60.
See https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script for more information about build script outputs.

riscv/build.rs Outdated
@@ -1,12 +1,16 @@
use std::env;

fn main() {
let target = env::var("TARGET").unwrap();
println!("cargo::rustc-check-cfg=cfg(riscv)");
println!("cargo::rustc-check-cfg=cfg(riscv32)");
Copy link
Contributor Author

@rmsyn rmsyn May 5, 2024

Choose a reason for hiding this comment

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

We might want to move these checks (riscv32 + riscv64) into their respective conditional branches.

All the checks pass, so I'm not 100% on how these checks are supposed to work.

Edit: I guess it works because of this line from the docs:

Note that all possible cfgs should be defined, regardless of which cfgs are currently enabled. This includes all possible values of a given cfg name.

Which probably means I should change the checks in riscv-rt and riscv-semihosting.

Copy link
Contributor

Choose a reason for hiding this comment

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

with cargo:rustc-check-cfg you just inform cargo that you might have a custom cfg item.

@romancardenas
Copy link
Contributor

I don't like the idea of bumping MSRV from 1.60 to 1.77 due to a nightly clippy issue... I looked at the line provided by your post and:

Note: The old invocation prefix cargo: (one colon only) is deprecated and won’t get any new features. To migrate, use two-colons prefix cargo::, which was added in Rust 1.77. If you were using cargo:KEY=VALUE for arbitrary links manifest key-value pairs, it is encouraged to switch to cargo::metadata=KEY=VALUE. Stick to cargo: only if the support of Rust version older than 1.77 is required.

I suggest using just : instead of :: until we face this issue in stable clippy.

Copy link
Contributor

@romancardenas romancardenas left a comment

Choose a reason for hiding this comment

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

Let's see if we can get rid of bumping the MSRV

riscv/build.rs Outdated Show resolved Hide resolved
riscv-rt/build.rs Outdated Show resolved Hide resolved
@jsgf jsgf mentioned this pull request May 7, 2024
Uses new lint checks for `cfg` variables.
rmsyn added 2 commits May 7, 2024 04:29
Uses new lint checks for `cfg` variables.
@rmsyn
Copy link
Contributor Author

rmsyn commented May 7, 2024

I don't like the idea of bumping MSRV from 1.60 to 1.77 due to a nightly clippy issue

I didn't like it either. Looks like the single colon fixed the warning/error. Weird that the compiler would suggest the non-MSRV compatible fix... Probably pretty involved for it to do otherwise.

Either way, the CI looks like it's fixed now. Thank you for the suggestion @romancardenas.

Copy link
Contributor

@romancardenas romancardenas left a comment

Choose a reason for hiding this comment

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

LGTM

@romancardenas romancardenas added this pull request to the merge queue May 7, 2024
Merged via the queue into rust-embedded:master with commit 8e148af May 7, 2024
95 checks passed
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.

4 participants