Skip to content

ci: add 'NO_INSTALL' flag to ci/rust-version.sh #6663

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.

Sign up for GitHub

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

Merged
yihau merged 1 commit into from
Jun 27, 2025
Merged

ci: add 'NO_INSTALL' flag to ci/rust-version.sh #6663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci/docker/env.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
ci_docker_env_sh_here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"

# shellcheck disable=SC1091
source "${ci_docker_env_sh_here}/../rust-version.sh"
NO_INSTALL=1 source "${ci_docker_env_sh_here}/../rust-version.sh"

if [[ -z "${rust_stable}" || -z "${rust_nightly}" ]]; then
echo "Error: rust_stable or rust_nightly is empty. Please check rust-version.sh." >&2
Expand Down
4 changes: 4 additions & 0 deletions ci/rust-version.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ export rust_stable="$stable_version"

export rust_nightly=nightly-"$nightly_version"

if [[ -n $NO_INSTALL ]]; then
return
fi

[[ -z $1 ]] || (
Copy link
Copy Markdown

@willhickey willhickey Jun 24, 2025

Choose a reason for hiding this comment

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

Why does this code block even run, since ci/docker/env.sh sources this script with no args?

Copy link
Copy Markdown
Member Author

@yihau yihau Jun 24, 2025

Choose a reason for hiding this comment

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

yeah… maybe I should extract the env from the actual logic 🤔. actually, I’m in a bit of a dilemma. we’ve already advertised using ci/rust-version.sh to install https://github.com/anza-xyz/agave/blob/master/README.md?plain=1#L28. also, I think we’ve gotten used to updating the rust stable/nightly version in this file. splitting it might open a long thread 🙈 so I decide to just introduce a no harm env to control the flow

Copy link
Copy Markdown

@willhickey willhickey Jun 24, 2025

Choose a reason for hiding this comment

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

Yeah I'm fine with merging this as a workaround.

But I really don't see how this it's printing the ignoring unknown argument warning in this case, since that whole code block should be skipped if -z $1. Do you understand what's happening to make it print the warning?

Copy link
Copy Markdown
Member Author

@yihau yihau Jun 25, 2025

Choose a reason for hiding this comment

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

afaik, when we source a file, it shares the same context. if we don’t reset the args, they will be inherited by the script.

Copy link
Copy Markdown

@willhickey willhickey Jun 26, 2025

Choose a reason for hiding this comment

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

Ah yeah I just reproduced this locally... I didn't know sourced scripts picked up the args from the parent.


rustup_install() {
Expand Down