-
Notifications
You must be signed in to change notification settings - Fork 784
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
emit rustc-check-cfg only on rust 1.80+ #4168
Conversation
65c5ee4
to
d8f7aef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I thought I tested it when i added them in #4163, and didn't see anything. But I can reproduce this as well (and there also in the CI log), so I guess I'm misremembering or something 🤷 . Given that we now check the rustc
version we can maybe also switch to the cargo::
(double colon) variant, which is preferred since 1.77.
Ah, looks like cargo explicitly rejects that due to the lower msrv. I guess I'll drop that second commit and merge? |
pyo3-build-config/src/lib.rs
Outdated
} | ||
pieces.next()?.parse().ok() | ||
}) | ||
.clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a short-team solution yes, but I think Cargo is a bit overzealous here as they statically second-guess code which can adjust itself at runtime. Will the old format ever be phased out? What if the range between the current version and our MSRV saddles that release and we have to dynamically adjust? |
Agreed, I asked about this in rust-lang/cargo#13868 (comment) |
364a311
to
d8f7aef
Compare
d8f7aef
to
cb65548
Compare
Testing on MSRV just now I got a lot of build warnings on the console:
... so I guess let's gate these behind a version check.