introduce cargo-nightly

[KirillLykov]

Problem

Spin off PR for #27018

Summary of Changes

• Introduce ./cargo-nightly
• Replace ./cargo nightly with ./cargo-nightly

[dmakarov]

Why not place cargo-nightly under scripts/ subdirectory? It seems this script is only needed for CI or other scripts.

[KirillLykov]

Why not place cargo-nightly under scripts/ subdirectory? It seems this script is only needed for CI or other scripts.

The only reason why it is in the root is to make it easier for users to find it (so it is next to ./cargo). ./cargo cannot be moved to scripts because some clients might use it according to @mvines

[dmakarov]

Why not place cargo-nightly under scripts/ subdirectory? It seems this script is only needed for CI or other scripts.

The only reason why it is in the root is to make it easier for users to find it (so it is next to ./cargo). ./cargo cannot be moved to scripts because some clients might use it according to @mvines

exactly for this reason I think it should be moved to scripts/ because cargo-nightly is not meant to be used by end users but by other scripts and CI only, isn't this correct? Users should use the standard cargo installed by rustup.

[joncinque]

During local development, we have to use the right version of nightly when formatting and clippy-ing, so I'd prefer to keep this at the top-level. On the flip-side, I appreciate that there isn't a huge difference between running:

../cargo-nightly fmt

and

../scripts/cargo-nightly fmt

Looking at this from another angle -- how do you use the right version of nightly when you're developing? Maybe I'm doing something silly.

[mvines]

Does anybody recall why we use nightly for fmt? I never do in practice actually.

The initial use of nightly was just a crutch for, umm, some benches I think? Ideally only rely on stable so further promotion of nightly doesn't feel so good IMO

[yihau]

for nightly fmt PR, https://www.github.com/solana-labs/solana/pull/23244
will check is it okay to use stable fmt check.
btw, the coverage test need nightly as well. (grcov need)

[joncinque]

@mvines I think it was to make sure that the use statements were all in one, ie:

use {
    solana_crate::a,
    solana_more::b,
    solana_still::c
};

[mvines]

@mvines I think it was to make sure that the use statements were all in one, ie:

use {
    solana_crate::a,
    solana_more::b,
    solana_still::c
};

Ah, that sounds familiar. That feature is not stablized yet though?

[joncinque]

Ah, that sounds familiar. That feature is not stablized yet though?

Doesn't seem like it unfortunately:

$ ./cargo fmt --all
Warning: can't set `imports_granularity = One`, unstable features are only available in nightly channel.
Warning: can't set `group_imports = One`, unstable features are only available in nightly channel.

[mvines]

barf, ok

[ilya-bobyr]

There are a number of calls to

scripts/cargo-for-all-lock-files.sh nightly ...

Do they also need to be changed to be routed though cargo-nightly?
Or the expectation is that ./cargo-nightly would be an alias for ./cargo nightly?

As ./cargo nightly is already invoked by other scripts, I assume, this indicates complexity, meaning, moving forward it seems reasonable to see more indirect invocations, not less.
If toolchain selection is done via a call to a different script, every caller needs to be aware of the toolchain selection.
While, when it is done as an argument, scripts that just forward the arguments do not need to care.

[ilya-bobyr]

There is a well known(?) pattern used for this:

here=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )

From here: https://stackoverflow.com/a/246128/1989046
7.7k upvotes.

I can see that in other scripts the same here=$(dirname "$0") is used.
So, it is probably out of scope for this PR.

[KirillLykov]

might be a good idea to make a separate PR for this: I saw more than one way of doing this in the scripts (ci, scripts folders)

[mvines]

here=$(dirname "$0") is pretty common and it works fine except for cases when a script is expected to be sourced , in which case BASH_SOURCE is certainly needed.

[KirillLykov]

There are a number of calls to

scripts/cargo-for-all-lock-files.sh nightly ...

Do they also need to be changed to be routed though cargo-nightly? Or the expectation is that ./cargo-nightly would be an alias for ./cargo nightly?

As ./cargo nightly is already invoked by other scripts, I assume, this indicates complexity, meaning, moving forward it seems reasonable to see more indirect invocations, not less. If toolchain selection is done via a call to a different script, every caller needs to be aware of the toolchain selection. While, when it is done as an argument, scripts that just forward the arguments do not need to care.

cargo-for-all-lock-files.sh was modified in the parent PR. These changes are not part of this one to make it simpler to review and lower the risk of bugs. Will be addressed in the follow up PR

[KirillLykov]

@joncinque @mvines @dmakarov @ilya-bobyr @yihau have we reached consensus about these changes? If yes, please approve, these changes are prerequisite for follow up PRs

[joncinque]

I think it's a good change since I often do ./cargo nightly ... in my dev workflow, but please get approval from someone else before merging since I'm just one datapoint that spends less time in this repo.

[mvines]

Suggested change

	here=$(dirname "$0")
	here="$(dirname "$0")"

[mvines]

Please put this shellcheck directive directly above source "${here}"/ci/rust-version.sh nightly

[mvines]

We do have ./scripts/cargo-fmt.sh for prettying. What are some of the other reasons folks use ./cargo nightly?

[joncinque]

I also use it all the time for:

./cargo nightly clippy -Zunstable-options --all-targets -- --deny=warnings --deny=clippy::integer_arithmetic

[mvines]

kk, introducing a./cargo-nightly seems fine. we've bikeshedded this enough!

[ilya-bobyr]

Sorry if this is continuing the bike shedding, but why not keep ./cargo nightly as is?
It seems clear why cargo build is better than ./cargo build - it is just standard.
But passing the toolchain as an argument is also pretty standard.
And it has an advantage that tools that do not care about a specific toolchain, can just pass arguments to ./cargo without having a special case for "normal" vs "nightly" invocation.

I would imagine, cargo should support custom toolchain names, specified in rust-toolchain.toml, eventually.