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

Old syntax suggestion #13874

Merged
merged 5 commits into from
May 8, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Old syntax suggestion.
  • Loading branch information
torhovland committed May 7, 2024
commit 2607f6d30fd492fe0657cbd23b278cff7d8aadd3
18 changes: 16 additions & 2 deletions src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
@@ -725,13 +725,26 @@ impl BuildOutput {
fn check_minimum_supported_rust_version_for_new_syntax(
pkg_descr: &str,
msrv: &Option<RustVersion>,
key: &str,
) -> CargoResult<()> {
if let Some(msrv) = msrv {
let new_syntax_added_in = RustVersion::from_str("1.77.0")?;
if !new_syntax_added_in.is_compatible_with(msrv.as_partial()) {
let prefix = format!("{key}=");
weihanglo marked this conversation as resolved.
Show resolved Hide resolved

let old_syntax_suggestion = RESERVED_PREFIXES
Copy link
Contributor

@epage epage May 7, 2024

Choose a reason for hiding this comment

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

When it isn't in RESERVED_PREFIXES, if the key is metadata, we should suggest cargo:{value}

Unsure if we should say much more otherwise. The key is either not supported on their MSRV or they didn't realize they should use metadata= first with cargo::

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

.contains(&&*prefix)
.then(|| {
format!(
"Consider using the old `cargo:` syntax in front of `{prefix}`.\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

imo splitting up the suggestion like this feels a little more awkward to read.

Maybe

Suggested change
"Consider using the old `cargo:` syntax in front of `{prefix}`.\n"
"Switch to `cargo:{prefix}` (note the single colon).\n"

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 wanted to make it clear that the single colon is an old syntax. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Instead of splitting the full instruction to two places, I'd like to see it combined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, how about now?

)
})
.unwrap_or_default();

bail!(
"the `cargo::` syntax for build script output instructions was added in \
Rust 1.77.0, but the minimum supported Rust version of `{pkg_descr}` is {msrv}.\n\
{old_syntax_suggestion}\
{DOCS_LINK_SUGGESTION}"
);
}
@@ -793,9 +806,10 @@ impl BuildOutput {
};
let mut old_syntax = false;
let (key, value) = if let Some(data) = line.strip_prefix("cargo::") {
check_minimum_supported_rust_version_for_new_syntax(pkg_descr, msrv)?;
// For instance, `cargo::rustc-flags=foo` or `cargo::metadata=foo=bar`.
parse_directive(whence.as_str(), line, data, old_syntax)?
let (key, value) = parse_directive(whence.as_str(), line, data, old_syntax)?;
check_minimum_supported_rust_version_for_new_syntax(pkg_descr, msrv, key)?;
(key, value)
} else if let Some(data) = line.strip_prefix("cargo:") {
old_syntax = true;
// For instance, `cargo:rustc-flags=foo`.