x setup -h -v should work

[jyn514]

x setup takes a short list of paths determined at runtime. Currently, it's special-cased in bootstrap rather than going through Step::should_run. But there's no need to special-case it - it can do the run time check in should_run instead, there's no sandbox. Switching it to should_run also has the advantage of automatically fixing x setup -h -v.

The list of paths is "all toml files in src/bootstrap/defaults".

Mentoring instructions:

• Add impl Step for Profile in src/bootstrap/setup.rs
• Remove the special-casing for Subcommand::Setup in Builder::new and Builder::get_step_descriptions.

cc @aswild - are you interested in tackling this after #96003?
@rustbot label +A-rustbuild +E-easy +E-medium
cc #96003 (comment)

[jyn514]

@rustbot label -E-medium +E-mentor

[lameferret]

@rustbot claim

[lameferret]

@jyn514 can I get some more details please, as Profile is a lot different from the likes of docs.rs, native.rs, etc, and I can't figure it out looking at them.
Also, how to run tests for /bootstrap ?

[jyn514]

Hi @anuvratsingh, I left some suggestions on https://rust-lang.zulipchat.com/#narrow/stream/122652-new-members/topic/impl.20Step.20for.20Profile :)

[lameferret]

Wasn't able to resolve this, but here are some resources shared by jyn514

1. https://discord.com/channels/442252698964721669/487245758739906560/964730715222732800
2. https://rust-lang.zulipchat.com/#narrow/stream/122652-new-members/topic/impl.20Step.20for.20Profile

[bentongxyz]

HI, is it okay if I try to work on this?

@rustbot claim

[bentongxyz]

Hi @jyn514, I would like to ask you a few questions, hope you don't mind! I'll be very succinct.

Right now, I managed to impl Step for Profile, so that if i run ./x.py setup -h -v, it prints:

(...Redacted)

        --llvm-profile-generate 
                        generate PGO profile with llvm built for rustc
        --llvm-profile-use PROFILE
                        use PGO profile for llvm build

Available paths:
    ./x.py setup src/bootstrap/defaults/config.codegen.toml
    ./x.py setup src/bootstrap/defaults/config.compiler.toml
    ./x.py setup src/bootstrap/defaults/config.library.toml
    ./x.py setup src/bootstrap/defaults/config.tools.toml
    ./x.py setup src/bootstrap/defaults/config.user.toml

The paths under 'Available paths:' are dynamically generated by getting all toml files under 'defaults' folder.

However, I have trouble proceeding because the setup subcommand's current behaviour actually does not take a path. Instead, it takes a profile name, such as library. I quote the help section:

This subcommand accepts a 'profile' to use for builds. For example:
./x.py setup library

i.e. If I run ./x.py setup src/bootstrap/defaults/config.library.toml, it will return an error.

So, should I change the current behaviour so that it accepts a path, e.g. src/bootstrap/defaults/config.library.toml?

This would involve changes to, amongst other things,

• the setup subcommand taking paths: Vec<PathBuf> instead of profile: Profile here in flag.rs line 137;
• accepting path instead of profile here in flag.rs line 580;
• removing this in lib.rs line 659

I hope I did not misunderstood something!

P.S. I'd be happy to chat with you in rust zulipchat if you'd prefer.

[jyn514]

This looks great so far! Thanks for working on it :)

However, I have trouble proceeding because the setup subcommand's current behaviour actually does not take a path. Instead, it takes a profile name, such as library

This is a great point. I agree it should stay as the profile name. I think I mislead you in the original description, sorry - we already hard-code the possible profiles in enum Profile, so we can just loop through Profile::all() and register all those paths with should_run.alias(profile): https://rust-lang.zulipchat.com/#narrow/stream/122652-new-members/topic/impl.20Step.20for.20Profile/near/279118083

[bentongxyz]

Hi @jyn514, so i implemented should_run using .alias like so (excerpt):

...
    fn should_run(mut run: ShouldRun<'_>) -> ShouldRun<'_> {
        for choice in Profile::all() {
            run = run.alias(&choice.to_string());
        }
        run
    }
...

It works for profile codegen, user and tools, but failed the assertion below for compiler and library, because the paths do exist:

the assertion:

 assert!(
            !self.builder.src.join(alias).exists(),
            "use `builder.path()` for real paths: {}",
            alias
        );

I could make an exception for compiler and library for the assertion, though it does look a bit ugly.

Do you think I should do the exception, or could there be better way?

[jyn514]

@bentongxyz I think just removing the assertion altogether is ok.

[bentongxyz]

Hi @jyn514, I have made PR #96584, please review at your convenience, much thanks!

[Noratrieb]

Looks like this was fixed now, closing as completed.