Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Install xargo using CI dictated cargo version if available#9068

Merged
jackcmay merged 12 commits intosolana-labs:masterfrom
jackcmay:xargo-version
Mar 26, 2020
Merged

Install xargo using CI dictated cargo version if available#9068
jackcmay merged 12 commits intosolana-labs:masterfrom
jackcmay:xargo-version

Conversation

@jackcmay
Copy link
Copy Markdown
Contributor

@jackcmay jackcmay commented Mar 25, 2020

Problem

The SDK install script attempts to install xargo which is required to build Rust BPF programs. The script uses whatever version of cargo is installed on the host machine to do so. in CI the version of cargo is always explicit and the machine's default toolchain is not updated, consequently the CI machines default version is very old. This old version does not support installing the latest version of xargo.

Summary of Changes

  • Use the CI version of cargo if the CI defined cargo version environment variable is defined (rust_stable.
  • cargo install now installs the latest version of a package so remove the use of cargo install-update

Fixes #

@jstarry
Copy link
Copy Markdown
Contributor

jstarry commented Mar 26, 2020

FYI looks like cargo install xargo will now update xargo to the latest version and will do nothing if it's already installed. This was released as part of 1.41.0: rust-lang/cargo#7560 So we can rip out cargo-update

@jackcmay
Copy link
Copy Markdown
Contributor Author

@jstarry Cool, I'll both set the stable version as the default toolchain and rip out update and replace with cargo install

@jstarry
Copy link
Copy Markdown
Contributor

jstarry commented Mar 26, 2020

@jackcmay, @mvines just added a note in #devops about setting the default toolchain being disruptive for devs, so we should be careful about that. Maybe we could change the command to cargo +stable to avoid that?

Comment thread ci/rust-version.sh
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 26, 2020

Codecov Report

Merging #9068 into master will increase coverage by <.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #9068     +/-   ##
========================================
+ Coverage    80.4%   80.4%   +<.1%     
========================================
  Files         268     268             
  Lines       58796   58796             
========================================
+ Hits        47320   47326      +6     
+ Misses      11476   11470      -6

@jackcmay
Copy link
Copy Markdown
Contributor Author

Agreed, not going to set default toolchain

set -e
cargo install-update -i xargo
set -ex
cargo +"${rust_stable:-}" install xargo
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mvines know any cool bash tricks to improve this?

set -e
cargo install-update -i xargo
set -ex
cargo +"${rust_stable:-}" install xargo
Copy link
Copy Markdown
Contributor

@mvines mvines Mar 26, 2020

Choose a reason for hiding this comment

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

I'd just go with a simple approach:

Suggested change
cargo +"${rust_stable:-}" install xargo
if [[ -n $rust_stable ]]; then
cargo +"$rust_stable" install xargo
else
cargo install xargo
fi

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

shellcheck does not like this since it can't tell if rust_stable is defined (rust_stable should probably be in all caps) and would rather not have to clutter up with a spellcheck "allow" statement

@jackcmay jackcmay changed the title Recreate xargo update issue Install xargo using CI dictated cargo version if available Mar 26, 2020
@jackcmay jackcmay merged commit 30bed18 into solana-labs:master Mar 26, 2020
@jackcmay jackcmay deleted the xargo-version branch March 26, 2020 18:47
@jackcmay jackcmay added the v1.0 label Mar 26, 2020
mergify Bot pushed a commit that referenced this pull request Mar 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants