Skip to content

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

Merged
yihau merged 1 commit intoanza-xyz:masterfrom
yihau:ci-improve-output-message
Jun 27, 2025
Merged

ci: add 'NO_INSTALL' flag to ci/rust-version.sh#6663
yihau merged 1 commit intoanza-xyz:masterfrom
yihau:ci-improve-output-message

Conversation

@yihau
Copy link
Copy Markdown
Member

@yihau yihau commented Jun 20, 2025

Problem

there is a confusing message in our build:
Screenshot 2025-06-20 at 20 32 48

the message comes from

echo "$0: Note: ignoring unknown argument: $1" >&2

Summary of Changes

add an option, NO_INSTALL, in the script

@yihau yihau force-pushed the ci-improve-output-message branch from 6afb1f0 to a795385 Compare June 20, 2025 07:24
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.3%. Comparing base (8b6d80f) to head (a795385).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6663   +/-   ##
=======================================
  Coverage    83.3%    83.3%           
=======================================
  Files         849      849           
  Lines      379469   379469           
=======================================
+ Hits       316234   316266   +32     
+ Misses      63235    63203   -32     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yihau yihau marked this pull request as ready for review June 20, 2025 12:35
@yihau yihau requested review from mircea-c and willhickey June 20, 2025 12:35
Comment thread ci/rust-version.sh
return
fi

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

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

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

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

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

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.

@yihau yihau merged commit eb4306a into anza-xyz:master Jun 27, 2025
33 checks passed
@yihau yihau deleted the ci-improve-output-message branch June 27, 2025 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants