Rename toolchain#6981
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6981 +/- ##
=======================================
Coverage 83.2% 83.2%
=======================================
Files 853 853
Lines 375646 375646
=======================================
+ Hits 312626 312690 +64
+ Misses 63020 62956 -64 🚀 New features to boost your workflow:
|
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks good! I wanted to make sure this wouldn't break nix-style builds, and I think it'll be ok
| let toolchain_name = generate_toolchain_name(validated_toolchain_version.as_str()); | ||
| let toolchain_argument = format!("+{toolchain_name}"); |
There was a problem hiding this comment.
nit: Can you move these lines under the !config.no_rustup_override branch? generate_toolchain_name won't work properly in nix-like environments
There was a problem hiding this comment.
I'll see what I can do. generate_toolchain_name is outside the if condition because cargo_build_args requires a &str not a String.
Why do you think it won't work correctly? Maybe we could address that instead.
There was a problem hiding this comment.
In nix environments, there are a few considerations:
- rustup doesn't work, so any invocation of cargo with a toolchain fails
- there's no concept of
$HOME
The logic to find the toolchain assumes that things are stored under $HOME, so we should avoid doing that if someone doesn't want to install tools. And if someone doesn't want to use rustup, we shouldn't use any commands that have cargo +{toolchain_name}, and assume that the default rust can compile solana programs.
You can see how cargo-build-sbf is wrapped at https://github.com/arijoon/solana-nix/blob/87bea8cac979d14c758c24d2b9178c44a6e95b39/solana-cli.nix#L132
There was a problem hiding this comment.
Not yet the solution for nix, but I moved the function to inside the if condition: #7024.
5 participants
Problem
As pointed by @febo, having only
+solanafor the toolchain name can be confusing, since it does not inform us which rust version we are running nor the platform tools version.Summary of Changes
From now on, we are going to link the toolchain as
<rust_version>-sbpf-solana-<platform_tools_version>. The currently released will be linked as1.84.1-sbpf-solana-v1.50.