-
Notifications
You must be signed in to change notification settings - Fork 4
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
Improves CI, fixes no_std
and other misc issues discovered
#51
Conversation
@asmello i copied over jonhoo/rust-ci-conf and one of the checks that's failing is miri. Could you look at the output when you get a chance please? |
There are two failing checks that I do not know how to resolve: Miri
cargo testThis one's weird. I don't encounter it locally.
|
Will look into the miri report, could be a legitimate find, could be a limitation of its unsafe lifetime checker. As for the other one, I think it's because |
Second one was solved by specifying the most recent versions of |
Ah, I forgot I also switched to I'm guessing neither change (using most recent versions and not using |
Yeah the miri thing goes deep into Rust intrinsics. I have a few theories, having to do with how Rust (or miri) tracks borrows for a given memory span. I suspect the borrows are tagged at the start of the span, and by shifting the pointer we invalidate the tag that was tracking its lifetime. Not sure how something like Probably the easiest solution is to avoid the pointer arithmetics. I have a few alternatives I want to try that should be just as performant, and lean more heavily on composition over |
I think using the most recent version is fine as a workaround, since we already have those as optional dependencies regardless. But for future reference, I believe the way to get around that error otherwise would be to add a section like this to the cargo manifest: [target.'cfg(any())'.dependencies]
serde = { version = "x.y.z", default-features = false, optional = true } The trick being that this forces EDIT: BTW we can probably switch back to |
Hah, I saw a comment by Jon in It must not have been The only crate which would make sense to me at the moment is edit: Also, miri keeps hanging now. |
Miri is quite slow, we may need to tune it to only run for a subset of tests or not at all unfortunately... Let me see if I can find out which dependency |
Nah don’t worry about the dep. I’ll figure it out. You’ve got the unsafe
bit which I can’t help with at all right now.
…On Mon, Jul 1, 2024 at 5:20 PM André Mello ***@***.***> wrote:
Miri is quite slow, we may need to tune it to only run for a subset of
tests or not at all unfortunately...
Let me see if I can find out which dependency quickcheck is failing on
(it's not the end of the world to use the direct variant if not).
—
Reply to this email directly, view it on GitHub
<#51 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIGFZCLFNGJ2AXAHZWDXLLZKHBY7AVCNFSM6AAAAABKETGMO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBRGA4TENZTHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I think that's exactly right, the CI error does point to that crate (sorry if I confused you by saying it was I think this is what we need: syn = { version = "1.0.109", optional = true } |
Yay, it worked! Also relevant: BurntSushi/quickcheck#317 I guess little chance we can get the fix pushed upstream, but we can try... Should be a trivial patch version bump... |
Rock on! I don't understand why we have to be explicit here though. Why is the resolver introducing syn = { version = "1.0", features = ["full"] } EditI didn't mean to seem dubious about the solution. I'm stoked about it! That was just my general confusion being expressed. Thanks for solving it. |
no_std
no_std
and other misc issues discovered
I'm going to go ahead and merge this in. |
It's the other way around! The The workaround, on our end, is to specify the smallest version of
No worries, man, it didn't read like that. 👍 |
Ah! I was under the impression it was picking up 2.0, assuming that 1.0 -> 2.0 introduced the breaking change. I didn't anticipate the breaking change having occurred somewhere between Thank you for the explanation! Edit: actually, wow, I just realized that this isn't the result of him changing versions. Merely that he needs to specify exactly I'm definitely going to be more meticulous about selecting explicit versions from now on. |
This! Yeah it's a really easy pitfall... That's why Jon recommends this check in CI, even though it can be annoying due to transitive problems. It's a sign of brokenness with cargo, but in general I've found that specifying the full version down to patch is the best approach, as dependabot can ensure we still receive patch updates automatically. |
no_std
& other remnants discovered bycargo hack
&cargo miri
0.5.0
in prep of release to satisfy semver check#[must_use]
, allowsclippy::must_use_candidate
IndexError
Cargo.lock
serde_json
version1.0.203
serde
version1.0.203
Co-authored-by: @asmello