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

cargo:: syntax should be permitted despite MSRV with the right configuration #13868

Closed
joshlf opened this issue May 6, 2024 · 14 comments · Fixed by #13869 or #13874
Closed

cargo:: syntax should be permitted despite MSRV with the right configuration #13868

joshlf opened this issue May 6, 2024 · 14 comments · Fixed by #13869 or #13874
Labels
A-build-scripts Area: build.rs scripts A-diagnostics Area: Error and warning messages generated by Cargo itself. E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@joshlf
Copy link

joshlf commented May 6, 2024

Currently, zerocopy's build.rs script contains the following:

    for version_cfg in version_cfgs {
        if rustc_version >= version_cfg.version {
            println!("cargo:rustc-cfg={}", version_cfg.cfg_name);
        }
    }

We have a lot of cfgs that are now triggering the recently-added unexpected_cfgs lint. In order to fix this, I added the following to our build script:

    if rustc_version >= (Version { major: 1, minor: 77, patch: 0 }) {
        for version_cfg in &version_cfgs {
            println!("cargo::rustc-check-cfg=cfg({})", version_cfg.cfg_name);
        }
    }

Note that this is gated to only run on Rust version 1.77 or later, since that's when the cargo:: syntax was introduced.

Unfortunately, this causes the following error to be emitted:

error: the `cargo::` syntax for build script output instructions was added in Rust 1.77.0, but the minimum supported Rust version of `zerocopy v0.8.0-alpha.9 (.../zerocopy)` is 1.56.0.
See https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script for more information about build script outputs.

This is spurious since our version detection logic will ensure that we don't emit these annotations prior to 1.77, but since it's a hard error, there's no way to work around it.

It would be great if there were a way to ask Cargo to disable this error for cases like these. The alternative for us is to just add a blanket #![allow(unexpected_cfgs)], which would be a shame since that lint is genuinely useful.

(I will also note that this error seems especially odd given that pre-1.77 toolchains simply ignore cargo::; it doesn't cause a compilation error or even a warning)

@fmease
Copy link
Member

fmease commented May 6, 2024

Strictly Cargo-related.

@rustbot transfer cargo

@rustbot rustbot transferred this issue from rust-lang/rust May 6, 2024
@weihanglo
Copy link
Member

Does the single colon version work for you? (cargo:rustc-check-cfg specifically).

@weihanglo weihanglo added A-build-scripts Area: build.rs scripts S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. and removed needs-triage labels May 6, 2024
@joshlf
Copy link
Author

joshlf commented May 6, 2024

Huh yeah that works, thanks! Maybe the lint could suggest the single-colon version instead? Or maybe the MSRV error could note the single-colon version as an alternative in its help text?

@weihanglo
Copy link
Member

They are all good suggestions!

@Urgau any opinion on this? Should we mention only single-colon syntax everywhere to avoid compatibility hazard?

@weihanglo weihanglo added A-diagnostics Area: Error and warning messages generated by Cargo itself. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed S-needs-info Status: Needs more info, such as a reproduction or more background for a feature request. labels May 6, 2024
@Urgau
Copy link
Member

Urgau commented May 6, 2024

I don't know if we should mention it in the diagnostic output it's already quite heavy; also mentioning the single column wouldn't be useful for the majority of users.

However I think like in the blog post we could mention it in the documention of rustc-check-cfg in the Cargo book.

@joshlf
Copy link
Author

joshlf commented May 6, 2024

Maybe it could be mentioned in the MSRV error, since that's a case where you know for a fact that it's relevant to the user? Ie, an error that says something like "note that you can get this to work by using cargo: instead of cargo::"?

@weihanglo
Copy link
Member

Maybe it could be mentioned in the MSRV error, since that's a case where you know for a fact that it's relevant to the user? Ie, an error that says something like "note that you can get this to work by using cargo: instead of cargo::"?

I am open to this. We could emit such a suggestion if the given instruction is one of the prefix in this list:

const RESERVED_PREFIXES: &[&str] = &[
"rustc-flags=",
"rustc-link-lib=",
"rustc-link-search=",
"rustc-link-arg-cdylib=",
"rustc-cdylib-link-arg=",
"rustc-link-arg-bins=",
"rustc-link-arg-bin=",
"rustc-link-arg-tests=",
"rustc-link-arg-benches=",
"rustc-link-arg-examples=",
"rustc-link-arg=",
"rustc-cfg=",
"rustc-check-cfg=",
"rustc-env=",
"warning=",
"rerun-if-changed=",
"rerun-if-env-changed=",
];

@weihanglo weihanglo reopened this May 6, 2024
@weihanglo weihanglo added E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. labels May 6, 2024
@torhovland
Copy link
Contributor

I'll look into this.

@mqudsi
Copy link

mqudsi commented May 8, 2024

Just want to point out that the docs are misleading, as they say

The old invocation prefix cargo: (one colon only) is deprecated and won’t get any new features.

implying that this switchover happened with 1.77 when support for the cargo:: prefix shipped. As mentioned in this issue, this is a problem since specifying an msrv in Cargo.toml causes hard compilation errors if you use a newer cargo with a pre-1.77 msrv but this error with rustc-check-cfg only happens under 1.80+ so it makes no sense to complain about it if you're using a rust version prior to that.

But the good news is that (at least with 1.80 nightly), cargo:rustc-check-cfg works even though the docs imply that it requires cargo::.

@epage
Copy link
Contributor

epage commented May 8, 2024

That is an oddity from the order of things being implemented compared to the order they were stabilized.

@mqudsi
Copy link

mqudsi commented May 8, 2024

I assumed as much (it’s why I thought to try it to begin with), but I’m grateful for the workaround all the same!

@bors bors closed this as completed in 1fec089 May 8, 2024
@davidhewitt
Copy link

We just ran into this same problem in PyO3. Will there ever be a point where the deprecated cargo: syntax will be removed?

We typically have quite low MSRV (currently 1.63) and are worried that we might reach a state where we cannot use cargo:: syntax due to the hard error and cannot use cargo: syntax because it has been removed.

@weihanglo
Copy link
Member

Will there ever be a point where the deprecated cargo: syntax will be removed?

No it hasn't been and will not be removed.
(at least won't be removed in a incompatible way)

check-cfg is in a weird state because of the stabilization order, though it should be generally fine with a single colon. Anything didn't work as expected?

@davidhewitt
Copy link

All working ok, for now we're just sticking with cargo: single colon and will update in the future. Thanks for confirming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts A-diagnostics Area: Error and warning messages generated by Cargo itself. E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
9 participants