Skip to content
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.

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

1.28.0 does not accept components separated by spaces #4220

Open
2 tasks done
Tracked by #4231
marmeladema opened this issue Mar 4, 2025 · 17 comments
Open
2 tasks done
Tracked by #4231

1.28.0 does not accept components separated by spaces #4220

marmeladema opened this issue Mar 4, 2025 · 17 comments
Labels
Milestone

Comments

@marmeladema
Copy link

Verification

Problem

Hello,

At work, we use

curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- --no-modify-path -y --default-toolchain nightly-2025-02-11 -c cargo rustc rust-src

to install a specific version of rust in our CI environment but since latest version (ie: v1.28.0), it fails with:

$ curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- --no-modify-path -y --default-toolchain nightly-2025-02-11 -c cargo rustc rust-src
info: downloading installer
error: error: unexpected argument 'rustc' found

Usage: rustup-init[EXE] [OPTIONS]

For more information, try '--help'.

After reading https://blog.rust-lang.org/2025/03/02/Rustup-1.28.0.html I understand that some amount of backward incompatible changes have been released (with very short notice) but I don't understand:

  1. Why it impacts my current workflow since I am specifically asking for a toolchain
  2. How to fix it. I am absolutely able to fix my CI configuration, but at the moment, I have no idea how and the error message doesn't provide any kind of help.

Thank you in advance for your help 👍

Steps

curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- --no-modify-path -y --default-toolchain nightly-2025-02-11 -c cargo rustc rust-src

Possible Solution(s)

No response

Notes

No response

Rustup version

1.28.0

Installed toolchains

nightly-2025-02-11

OS version

Linux
@marmeladema marmeladema added the bug label Mar 4, 2025
@marmeladema
Copy link
Author

I tried changing my CI to:

curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- --no-modify-path -y --default-toolchain none
source $HOME/.cargo/env
rustup toolchain install nightly-2025-02-11 -c cargo rustc rust-src

But the last command still fails with:

error: invalid value 'rustc' for '[TOOLCHAIN]...': invalid toolchain name: 'rustc'

@marmeladema
Copy link
Author

I finally figured it out. The problem is that the new version of rustup expect a comma-separated list of components whereas before, a space separated list at the end of the command line worked. Here is the fix:

curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- --no-modify-path -y --default-toolchain nightly-2025-02-11 -c cargo,rustc,rust-src

I guess this is just another breaking change, but I re-read multiple times https://blog.rust-lang.org/2025/03/02/Rustup-1.28.0.html and it is not announced anywhere?

@djc
Copy link
Contributor

djc commented Mar 4, 2025

Ah yes, this is likely an artifact of our clap upgrade that might not have been as much of a conscious change. Thanks for figuring it out, we will check how to mitigate this.

@marmeladema
Copy link
Author

@djc problem is resolved on my side but i'll leave it up to the rustup team to decide whether to close the issue or not.
The fix is easy, but not that easy to figure out so even if closed, a cheatsheet for common problems with 1.28.0 is probably helpful.

@lnicola

This comment has been minimized.

@BenjaminBrienen
Copy link

Maybe rustup could have some pre-release tests in an action that run rustup.exe in various ways to make sure the CLI interface doesn't break.

@rami3l
Copy link
Member

rami3l commented Mar 5, 2025

@djc djc changed the title v1.28.0 broke installing rust in CI 1.28.0 does not accept components separated by spaces Mar 5, 2025
@djc
Copy link
Contributor

djc commented Mar 5, 2025

We're also seeing some breakage for rustup component add --toolchain nightly rustfmt, but only on Windows:

error: invalid value 'C:\Users\runneradmin\.cargo\bin\rustup.exe' for '[+toolchain]': error: "C:\Users\runneradmin\.cargo\bin\rustup.exe" is not a valid subcommand, so it was interpreted as a toolchain name, but it is also invalid. To override the toolchain using the 'rustup +toolchain' syntax, make sure to prefix the toolchain override with a '+'

It's similar, but not the same as marmeladema's error from above.

EDIT: this fixed it:

- rustup toolchain add nightly --profile minimal
- rustup component add --toolchain nightly rustfmt
+ rustup toolchain install nightly --profile minimal --component rustfmt

Should potentially file a separate issue for this -- edited the title for this issue to keep the scope narrow.

@rami3l
Copy link
Member

rami3l commented Mar 5, 2025

I highly doubt whether we can fix this without regressing #4073, but if you can, using commas would be a better option for a quick fix.

cmichi added a commit to use-ink/docker-images that referenced this issue Mar 5, 2025
@rami3l
Copy link
Member

rami3l commented Mar 7, 2025

The expected behavior on our side is, of course:

<CMD> -c a,b d     =>  args=[d] c=[a, b]
<CMD> -c a -c b d  =>  args=[d] c=[a, b]
<CMD> d -c a b     =>  args=[d] c=[a, b]
<CMD> -c a b d     =>  args=[]  c=[a, b, d]

However it's all about figuring out the right usage of clap, I believe.

@djc
Copy link
Contributor

djc commented Mar 7, 2025

The expected behavior on our side is, of course:

<CMD> -c a,b d     =>  args=[d] c=[a, b]
<CMD> -c a -c b d  =>  args=[d] c=[a, b]
<CMD> d -c a b     =>  args=[d] c=[a, b]
<CMD> -c a b d     =>  args=[]  c=[a, b, d]

However it's all about figuring out the right usage of clap, I believe.

@epage if you have any suggestions on whether/how this is possible, would be great!

@epage
Copy link

epage commented Mar 7, 2025

I've tried to emulate the different behaviors to see how they stack up with each other

rustup 1.25.0
#!/usr/bin/env nargo
---
[dependencies]
clap = "2"
---

fn main() {
    let app = clap::App::new("foo")
        .arg(
            clap::Arg::with_name("toolchain")
                .required(true)
                .multiple(true),
        )
        .arg(
            clap::Arg::with_name("profile")
                .long("profile")
                .takes_value(true)
                .required(false),
        )
        .arg(
            clap::Arg::with_name("components")
                .help("Add specific components on installation")
                .long("component")
                .short("c")
                .takes_value(true)
                .multiple(true)
                .use_delimiter(true),
        );
    let m = app.get_matches();
    dbg!(&m);
}
rustup 1.26.0
#!/usr/bin/env nargo
---
[dependencies]
clap = "3"
---

fn main() {
    let app = clap::App::new("foo")
         .arg(
             clap::Arg::new("toolchain")
                 .required(true)
                 .takes_value(true)
                 .multiple_values(true),
         )
         .arg(
             clap::Arg::new("profile")
                 .long("profile")
                 .takes_value(true),
         )
         .arg(
             clap::Arg::new("components")
                 .help("Add specific components on installation")
                 .long("component")
                 .short('c')
                 .takes_value(true)
                 .multiple_values(true)
                 .use_value_delimiter(true)
             .action(clap::ArgAction::Append),
         );
    let m = app.get_matches();
    dbg!(&m);
}
rustup 1.27.1
#!/usr/bin/env nargo
---
[dependencies]
clap = "4"
---

fn main() {
    let app = clap::Command::new("foo")
         .arg(
             clap::Arg::new("toolchain")
                 .required(true)
                 .num_args(1..)
         )
         .arg(
             clap::Arg::new("profile")
                 .long("profile")
                 .num_args(1),
         )
         .arg(
             clap::Arg::new("components")
                 .help("Add specific components on installation")
                 .long("component")
                 .short('c')
                 .num_args(1..)
                 .use_value_delimiter(true)
             .action(clap::ArgAction::Append),
         );
    let m = app.get_matches();
    dbg!(&m);
}
rustup 1.28.0
#!/usr/bin/env nargo
---
[dependencies]
clap = { version = "4", features = ["derive"] }
---

use clap::Parser;

#[derive(Debug, Default, Parser)]
struct UpdateOpts {
    #[arg(
        num_args = 1..,
    )]
    toolchain: Vec<String>,

    #[arg(long)]
    profile: Option<String>,

    /// Comma-separated list of components to be added on installation
    #[arg(short, long, value_delimiter = ',')]
    component: Vec<String>,
}

fn main() {
    let m = UpdateOpts::parse();
    dbg!(&m);
}

Note: toolchain lost its required status in the port to clap_derive. A common mistake people make is they assume Option is the only way to make things optional but Vec does it as well because a Vec naturally represents storing 0 items.

Version -c a,b d -c a -c b d d -c a b -c a b d
1.25.2 t=d, c=a,b error t=d, c=a,b error
1.26.0 t=d, c=a,b error t=d, c=a,b error
1.27..1 error error t=d, c=a,b error
1.28.0 t=d, c=a,b t=d, c=a,b t=d,b, c=a t=d,b, c=a

btw if you make toolchain action=Append but num_args=1, then it won't allow splitting toolchain values around flags.

From my comment at #4073 (comment)

Historical context: clap v2 conflated flag "occurrences" with argument values as multiple. To get behavior people wanted, they had to opt-in to behavior they don't want and then know how to work around it. In clap v3, we split multiple into multiple_occurrences (generally what people want, now ArgAction::Append) and multiple_values (rarely wanted, now num_args(1..)).

Actually accepting multiple values per flag is a problematic case, see the warning in the docs. To support cases like -c a b d, the parser cannot be strictly greedy (is another value available? then its automatically part of -c) but needs a form of backtracking (backtrack on missing <toolchain> and give one less parameter to -c). Its possible to get that kind of backtracking behavior but it comes with its own downsides, especially in a general purpose parser (bpaf` is the only parser I'm aware that has backtracking but unsure how they handle the other trade offs).

The only way to address this with clap is to regress #4073. For me, a question is whether -c a b d and d -c a b was intentional or an accidental slip of behavior because of how convoluted clap v2's API was (needed a requires_delimiter(true) to opt-out of it). I remember being concerned about preserving behavior when upgrade Cargo to clap v3 and surprised to see they actually protected against it.

@rami3l
Copy link
Member

rami3l commented Mar 8, 2025

@epage Thanks for your detailed analysis.

Immediately after writing that comment I have realized there are a lot of ambiguities in my examples.
I have also contributed to clap's painstakingly long main parsing routine, and I definitely agree that parsing is hard.

For me, a question is whether -c a b d and d -c a b was intentional or an accidental slip of behavior

If we cannot make everyone happy and, if, as you said, we have already broken stuff when @djc migrated our project away from clap v2, I think it'd probably be reasonable to get back to the "old" behavior as in v1.26.0.

I probably would need more info on how this could be done because, as I migrated to clap-derive during the v1.28.0 release cycle, I was assuming that the previous migration was correct. (Not to deny @djc's hard work; we are all human beings after all 🙇)

@epage
Copy link

epage commented Mar 10, 2025

, I think it'd probably be reasonable to get back to the "old" behavior as in v1.26.0.

To be clear, that would regress #4073. Maybe my bias from seeing problems with num_args but I would seriously consider keeping the current behavior or at least switch to num_args=1 from toolchain so d -c a b clearly errors, rather than getting people upgrading getting weird behavior.

I probably would need more info on how this could be done because, as I migrated to clap-derive during the v1.28.0 release cycle, I was assuming that the previous migration was correct. (Not to deny @djc's hard work; we are all human beings after all 🙇).

I believe you would just add num_args=1.. to component to get the clap v2 behavior.

btw iirc num_args = 1.., is redundant for positionals, so that might be able to be removed

@crawfxrd
Copy link

  -c, --component <components>...  Add specific components on installation

The usage was never clearly defined.

But as the reporter of #4073, I have only ever used tooling that accepts -c a -c b -c d and -c a,b,d. I would never expect a named argument to accept a space separated list (-c a b d).

@rami3l rami3l added this to the 1.28.2 milestone Mar 14, 2025
@rami3l
Copy link
Member

rami3l commented Mar 14, 2025

@epage Thanks a lot! I think that's all the information I'll need for now.

The usage was never clearly defined.

@crawfxrd You are right... That's why I've changed the description to the following:

-c, --component <COMPONENT> Comma-separated list of components to be added on installation

@foresterre
Copy link
Contributor

If space separated components are convenient, you can still issue a toolchain install first, and then run rustup component add <component>..., which takes the components as positional arguments. This is what cargo-msrv does, for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants