Fix `x setup -h -v` should work #96584

bentongxyz · 2022-04-30T17:04:05Z

I have to convert profile to path and back in order to remove special-casing in bootstrap. I also check for dry_run so that config.toml and/ or .git/hooks/pre-push will not be created if --dry-run is specified.

Please help me see if this is ok, thanks alot!

jyn514

Thanks for tackling this! It looks like basically the right approach, I just have a few comments on the implementation.

I also noticed that the should_run step is never actually hit, because setup is still special-cased in Flags::parse. Is it possible to remove that special-casing? You should double check that x setup without arguments still works (you probably will need to add DEFAULT = true in the Step impl) and that x setup invalid properly goes through the "no paths matched" path instead of "called unwrap on None".

One super cool thing to do is to still keep the description of each path, like it has currently:

Arguments:
    This subcommand accepts a 'profile' to use for builds. For example:

        ./x.py setup library

    The profile is optional and you will be prompted interactively if it is not given.
    The following profiles are available:

        library: Contribute to the standard library
        compiler: Contribute to the compiler itself
        codegen: Contribute to the compiler, and also modify LLVM or codegen
        tools: Contribute to tools which depend on the compiler, but do not modify it directly (e.g. rustdoc, clippy, miri)
        user: Install Rust from source

That will probably require modifying ShouldRun to allow passing in a description for the alias, though - no need to tackle it just now if it's tricky.

r? @Mark-Simulacrum just so this has someone assigned, but happy to be in charge of the review.

src/bootstrap/flags.rs

src/bootstrap/setup.rs

bentongxyz · 2022-05-01T05:38:49Z

Hi @jyn514, quick question here

I am working on changing Setup to take a single path, but here at builder.rs line 715, I have trouble converting single path to &[PathBuf] to be compatible with other match arms type:

If I do Subcommand::Setup { ref path } => (Kind::Setup, &[path.clone()])

Error: expected slice, found array of 1 element

If i do Subcommand::Setup { ref path } => (Kind::Setup, &[path.clone()][..])

Error: temporary value dropped while borrowed
creates a temporary which is freed while still in use

I am abit stuck here, what is the best way to approach this?

Thanks a bunch!

jyn514 · 2022-05-01T15:47:11Z

@bentongxyz I think it will work if you remove the call to clone: (Kind::Setup, &[path][..])

bentongxyz · 2022-05-02T06:13:51Z

@jyn514 still no good, if I do (Kind::Setup, &[path][..]), I got from the compiler:

expected struct std::path::PathBuf, found &std::path::PathBuf

bentongxyz · 2022-05-02T09:39:43Z

@jyn514 still no good, if I do (Kind::Setup, &[path][..]), I got from the compiler:

expected struct std::path::PathBuf, found &std::path::PathBuf

Thank you for your review!
Except for the above part, I have updated the code.

One super cool thing to do is to still keep the description of each path, like it has currently:

....

That will probably require modifying ShouldRun to allow passing in a description for the alias, though - no need to tackle it just now if it's tricky.

If this is ok, maybe this could be the goal for my next PR? I am looking for further work to do for the project too :)

jyn514 · 2022-05-02T23:48:34Z

@bentongxyz try this:

            Subcommand::Setup { ref path } => (Kind::Setup, if let Some(p) = path { std::slice::from_ref(p) } else { &[] }),

(this assumes path is an Option - we should still allow x setup without arguments.)

If this is ok, maybe this could be the goal for my next PR? I am looking for further work to do for the project too :)

Sounds great! thanks for working on this :)

@rustbot label +A-rustbuild +S-waiting-on-author +C-cleanup

src/bootstrap/builder.rs

rustbot · 2022-05-03T04:01:39Z

Error: The "Ready" shortcut only works on pull requests.

Please let @rust-lang/release know if you're having trouble with this bot.

jyn514 · 2022-05-03T04:01:47Z

@rustbot ready

src/bootstrap/setup.rs

bentongxyz · 2022-05-03T06:39:18Z

r? @Mark-Simulacrum

bors · 2022-05-09T00:03:12Z

☔ The latest upstream changes (presumably #96846) made this pull request unmergeable. Please resolve the merge conflicts.

bentongxyz · 2022-05-09T07:28:34Z

Hi @jyn514, in the meantime, please let me know if there is anything that I can work on :)

jyn514 · 2022-05-09T12:05:24Z

Hi @bentongxyz - #86890 is a good issue to try out (it's a little harder to test than this one, but not too bad), or you could go with the first approach I mentioned in #96584 (comment) so you don't have to wait for my PR to land.

@rustbot label -S-waiting-on-review +S-blocked

jyn514

Looks great! Thanks for the PR :)

src/bootstrap/setup.rs

bentongxyz · 2022-05-09T18:15:15Z

#86890 is a good issue to try out (it's a little harder to test than this one, but not too bad), or you could go with the first approach I mentioned in #96584 (comment) so you don't have to wait for my PR to land.

@jyn514 Thank you! I will take a look at #86890.

Meanwhile, I made changes to impl Step for Profile so it is more explicit that it is a temporary hack (reference).

See if it is okay for merge without needing #96501 to land first.

I can also help refactor it after #96501 is done!

Dylan-DPC · 2022-11-08T16:29:10Z

Thanks. We don't accept merge commits on this repo so kindly unmerge it and rebase it instead.

bentongxyz · 2022-11-08T17:39:36Z

You marked this as resolved, but I still see multiple paths - did you run into trouble?

some changes were reverted following this comment #96584 (comment) , I have changed back to single path, please see the new commit :)

bentongxyz · 2022-11-08T17:48:01Z

@rustbot ready

jyn514 · 2022-11-17T02:57:33Z

@bors r+ rollup

Sorry for the long delay! This looks good :)

bors · 2022-11-17T02:57:35Z

📌 Commit b2bc65b has been approved by jyn514

It is now in the queue for this repository.

bors · 2022-11-17T02:57:36Z

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

… r=jyn514 Fix `x setup -h -v` should work r? `@jyn514` I have to convert profile to path and back in order to remove special-casing in bootstrap. I also check for `dry_run` so that `config.toml` and/ or `.git/hooks/pre-push` will not be created if `--dry-run` is specified. Please help me see if this is ok, thanks alot!

matthiaskrgr · 2022-11-17T17:27:39Z

@bors r- failed in a rollup
https://github.com/rust-lang/rust/actions/runs/3490494662/jobs/5841998899#step:25:226
#104541

jyn514 · 2022-11-17T18:19:01Z

@bentongxyz you need to rebase one last time, dry_run changed from a field to a function

bors · 2022-12-06T00:55:55Z

☔ The latest upstream changes (presumably #105328) made this pull request unmergeable. Please resolve the merge conflicts.

- Remove setup special-casing in Flags::parse

jyn514 · 2022-12-17T23:16:52Z

src/bootstrap/flags.rs

@@ -351,7 +351,7 @@ To learn more about a subcommand, run `./x.py <subcommand> -h`",

        // fn usage()
        let usage = |exit_code: i32, opts: &Options, verbose: bool, subcommand_help: &str| -> ! {
-            let config = Config::parse(&["build".to_string()]);
+            let config = Config::parse(&["setup".to_string()]);


this change is so x setup one two doesn't update submodules before showing the usage error

jyn514 · 2022-12-17T23:17:27Z

@bors r+

bors · 2022-12-17T23:17:29Z

📌 Commit fca8290 has been approved by jyn514

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#96584 (Fix `x setup -h -v` should work) - rust-lang#105420 (Remove dead code after destination propagation) - rust-lang#105844 (Make the x tool use the x and x.ps1 scripts) - rust-lang#105854 (remove redundant clone) - rust-lang#105858 (Another `as_chunks` example) - rust-lang#105870 (avoid .into() conversion to identical types) - rust-lang#105875 (don't destuct references just to reborrow) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bentongxyz mentioned this pull request Apr 30, 2022

x setup -h -v should work #96049

Closed

jyn514 reviewed Apr 30, 2022

View reviewed changes

src/bootstrap/flags.rs Outdated Show resolved Hide resolved

src/bootstrap/setup.rs Show resolved Hide resolved

src/bootstrap/setup.rs Outdated Show resolved Hide resolved

bentongxyz force-pushed the x-setup-h-v-should-work branch from 174a5c6 to 18c337e Compare May 2, 2022 09:32

jyn514 reviewed May 2, 2022

View reviewed changes

src/bootstrap/builder.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 3, 2022

jyn514 mentioned this pull request May 3, 2022

The "Ready" shortcut should work on pull request review comments rust-lang/triagebot#1614

Closed

This comment has been minimized.

Sign in to view

jyn514 suggested changes May 3, 2022

View reviewed changes

src/bootstrap/setup.rs Outdated Show resolved Hide resolved

rust-highfive assigned Mark-Simulacrum May 3, 2022

bentongxyz force-pushed the x-setup-h-v-should-work branch from 435a6ee to 27af695 Compare May 9, 2022 06:22

rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2022

jyn514 approved these changes May 9, 2022

View reviewed changes

src/bootstrap/setup.rs Outdated Show resolved Hide resolved

apiraino added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label May 23, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 8, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2022

matthiaskrgr mentioned this pull request Nov 17, 2022

Rollup of 11 pull requests #104541

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 17, 2022

Remove special cases for setup subcommand

fca8290

- Remove setup special-casing in Flags::parse

jyn514 force-pushed the x-setup-h-v-should-work branch from b2bc65b to fca8290 Compare December 17, 2022 23:12

jyn514 reviewed Dec 17, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 17, 2022

matthiaskrgr mentioned this pull request Dec 18, 2022

Rollup of 7 pull requests #105876

Merged

bors merged commit 3fb5d8e into rust-lang:master Dec 18, 2022

rustbot added this to the 1.68.0 milestone Dec 18, 2022

bentongxyz deleted the x-setup-h-v-should-work branch March 16, 2023 04:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `x setup -h -v` should work #96584

Fix `x setup -h -v` should work #96584

bentongxyz commented Apr 30, 2022

jyn514 left a comment

bentongxyz commented May 1, 2022

jyn514 commented May 1, 2022 •

edited

Loading

bentongxyz commented May 2, 2022

bentongxyz commented May 2, 2022

jyn514 commented May 2, 2022 •

edited

Loading

rustbot commented May 3, 2022

jyn514 commented May 3, 2022

This comment has been minimized.

bentongxyz commented May 3, 2022

bors commented May 9, 2022

bentongxyz commented May 9, 2022

jyn514 commented May 9, 2022

jyn514 left a comment

bentongxyz commented May 9, 2022

Dylan-DPC commented Nov 8, 2022

bentongxyz commented Nov 8, 2022

bentongxyz commented Nov 8, 2022

jyn514 commented Nov 17, 2022

bors commented Nov 17, 2022

bors commented Nov 17, 2022

matthiaskrgr commented Nov 17, 2022

jyn514 commented Nov 17, 2022

bors commented Dec 6, 2022

jyn514 Dec 17, 2022

jyn514 commented Dec 17, 2022

bors commented Dec 17, 2022

Fix x setup -h -v should work #96584

Fix x setup -h -v should work #96584

Conversation

bentongxyz commented Apr 30, 2022

jyn514 left a comment

Choose a reason for hiding this comment

bentongxyz commented May 1, 2022

jyn514 commented May 1, 2022 • edited Loading

bentongxyz commented May 2, 2022

bentongxyz commented May 2, 2022

jyn514 commented May 2, 2022 • edited Loading

rustbot commented May 3, 2022

jyn514 commented May 3, 2022

This comment has been minimized.

bentongxyz commented May 3, 2022

bors commented May 9, 2022

bentongxyz commented May 9, 2022

jyn514 commented May 9, 2022

jyn514 left a comment

Choose a reason for hiding this comment

bentongxyz commented May 9, 2022

Dylan-DPC commented Nov 8, 2022

bentongxyz commented Nov 8, 2022

bentongxyz commented Nov 8, 2022

jyn514 commented Nov 17, 2022

bors commented Nov 17, 2022

bors commented Nov 17, 2022

matthiaskrgr commented Nov 17, 2022

jyn514 commented Nov 17, 2022

bors commented Dec 6, 2022

jyn514 Dec 17, 2022

Choose a reason for hiding this comment

jyn514 commented Dec 17, 2022

bors commented Dec 17, 2022

Fix `x setup -h -v` should work #96584

Fix `x setup -h -v` should work #96584

jyn514 commented May 1, 2022 •

edited

Loading

jyn514 commented May 2, 2022 •

edited

Loading