-
Notifications
You must be signed in to change notification settings - Fork 217
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
bump rust to 1.80.1 / 2024-08-08 #2487
Conversation
they released a new patch. will use it |
looking at the latest 3 anchor releases (0.29.0, 0.30.0, 0.30.1), they all depend on time v0.3.29, and Rust 1.80.1 is not happy with this version: not sure if it's a good idea to just run anchor test with their master branch: 86848e3 cc @joncinque |
@yihau I think it's reasonable to use master until a release is tagged with the fix. We might be able to follow along at coral-xyz/anchor#3143 |
thank you for the confirmation @joncinque 🫶 I just submitted an issue as a reminder: #2577 |
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.
👻
could be done by a follow up pr but seems coverage build is broken... https://buildkite.com/anza/agave/builds/9332#01914c6e-206a-46bc-bcd5-bd45f56c72e2/6-12191:
i think we need make sure to use |
let me check the coverage issue 🤔 |
ci/docker/Dockerfile
Outdated
FROM ubuntu:20.04 | ||
FROM ubuntu:22.04 |
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.
This is a pretty big change. I think these should be in their own separate PR.
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.
oh, actually, it will only affect the tests that use Rust 1.80.1. This means v2.0 and v1.18 won’t be affected by these changes (and old master commits that are using Rust 1.79.0). do you still have concerns if the situation is as I described? to me, it's a really good chance to upgrade those things when we introduce a new rust version haha. just like a new start!
btw, I’m still struggling with the coverage tests 😢 I will keep posting here
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.
IMO if we can make the ubuntu upgrade separately from this PR, we should. Then we keep this PR just about the rust version bump.
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.
okay! let me try! I think I figured out the coverage issue. need to rebuild the image. will push commits and ping you!
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.
Good callout Brooks - I'm in favor of bumping the Ubuntu version, but agree that it should be external to this PR
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.
@yihau hmm, maybe i should have been more explicit at my suggestion of this: #2487 (comment)
i think we should use vendorized llvm from the rust-lang, instead of installing the official llvm manually.. this is easier and natually is sync with rust nightly more. in that way, bumping ubuntu (is good thing!) can be separated from this pr.
recent nightly's coverage data is incompatible with llvm 18. it only works now with llvm 19. currently coverage is run under nightly (ie using llvm 19), yet using llvm-profdata
from stable rustup toolchain (which is still using llvm 18).
$ rustup component add --toolchain 1.80.1-x86_64-unknown-linux-gnu llvm-tools-preview
info: downloading component 'llvm-tools'
info: installing component 'llvm-tools'
31.2 MiB / 31.2 MiB (100 %) 14.8 MiB/s in 2s ETA: 0s
$ rustup component add --toolchain nightly-2024-08-08-x86_64-unknown-linux-gnu llvm-tools-preview
info: downloading component 'llvm-tools'
info: installing component 'llvm-tools'
32.7 MiB / 32.7 MiB (100 %) 14.7 MiB/s in 2s ETA: 0s
$ /home/ryoqun/.rustup/toolchains/1.80.1-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-profdata --version
LLVM (http://llvm.org/):
LLVM version 18.1.7-rust-1.80.1-stable
Optimized build.
$ /home/ryoqun/.rustup/toolchains/nightly-2024-08-08-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-profdata --version
LLVM (http://llvm.org/):
LLVM version 19.1.0-rust-1.82.0-nightly
Optimized build.
After that, I think we need to just meddle with PATH only running the grcov
.
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.
(nevertheless, thanks for working on bumping rust. as i like to use latest and greatest rust things (yet not enough time...), i'd like to appreciate your efforts very much.)
sorry for the late. I pushed a new commit for fixing coverage test and rebased master for solving conflict. looks all good! it's ready for another review! 🫡 |
[lints.rust] | ||
unexpected_cfgs = { level = "warn", check-cfg = [ | ||
'cfg(target_feature, values("static-syscalls"))', | ||
] } |
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.
here
[workspace.lints.rust.unexpected_cfgs] | ||
level = "warn" | ||
check-cfg = [ | ||
'cfg(target_os, values("solana"))', | ||
'cfg(feature, values("frozen-abi", "no-entrypoint"))', | ||
'cfg(RUSTC_WITH_SPECIALIZATION)', | ||
'cfg(RUSTC_WITHOUT_SPECIALIZATION)', | ||
] | ||
|
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.
nit: this can be a follow up pr, but it seems it's prime time for us to finally move lints from scripts/cargo-clippy-nightly.sh
for that, i think this formatting is preferred:
[workspace.lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = [
...
] }
like this: https://github.com/anza-xyz/agave/pull/2487/files#r1718380391
@@ -7,6 +7,10 @@ homepage = "https://anza.xyz" | |||
license = "Apache-2.0" | |||
edition = "2021" | |||
|
|||
[workspace.lints.rust.unexpected_cfgs] |
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.
seems there's no notable abi-related changes in rust 1.79 and 1.80. that said, this is a few of my select cool things from the rust change log:
also, this new panic source seems not to be affecting us: |
# Check llvm path | ||
llvm_profdata="$(find "$(rustc +"$rust_nightly" --print sysroot)" -name llvm-profdata)" | ||
if [ -z "$llvm_profdata" ]; then | ||
echo "Error: couldn't find llvm-profdata. Try installing the llvm-tools component with \`rustup component add llvm-tools-preview --toolchain=$rust_nightly\`" | ||
exit 1 | ||
fi | ||
llvm_path="$(dirname "$llvm_profdata")" |
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.
❤️
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.
lgtm with nits.
🪖 |
Summary of Changes
no-entrypoint
to the workspace-level lint config.