Repository navigation
x setup -h -v should work #96049
Description
Activity
@rustbot claim
@jyn514 can I get some more details please, as
Profileis 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?Hi @anuvratsingh, I left some suggestions on https://rust-lang.zulipchat.com/#narrow/stream/122652-new-members/topic/impl.20Step.20for.20Profile :)
Reacted by lameferretWasn't able to resolve this, but here are some resources shared by jyn514
Reacted by jynHI, is it okay if I try to work on this?
@rustbot claim
Reacted by jynHi @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.tomlThe paths under 'Available paths:' are dynamically generated by getting all
tomlfiles under 'defaults' folder.However, I have trouble proceeding because the
setupsubcommand's current behaviour actually does not take a path. Instead, it takes a profile name, such aslibrary. I quote the help section:This subcommand accepts a 'profile' to use for builds. For example:
./x.py setup libraryi.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
setupsubcommand takingpaths: Vec<PathBuf>instead ofprofile: Profilehere 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.
- the
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 throughProfile::all()and register all those paths withshould_run.alias(profile): https://rust-lang.zulipchat.com/#narrow/stream/122652-new-members/topic/impl.20Step.20for.20Profile/near/279118083Hi @jyn514, so i implemented
should_runusing.aliaslike 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,userandtools, but failed the assertion below forcompilerandlibrary, because the paths do exist:assert!( !self.builder.src.join(alias).exists(), "use `builder.path()` for real paths: {}", alias );
I could make an exception for
compilerandlibraryfor the assertion, though it does look a bit ugly.Do you think I should do the exception, or could there be better way?
@bentongxyz I think just removing the assertion altogether is ok.
Reacted by Benjamin TongLooks like this was fixed now, closing as completed.
Reacted by jyn
x setuptakes a short list of paths determined at runtime. Currently, it's special-cased in bootstrap rather than going throughStep::should_run. But there's no need to special-case it - it can do the run time check inshould_runinstead, there's no sandbox. Switching it toshould_runalso has the advantage of automatically fixingx setup -h -v.The list of paths is "all toml files in
src/bootstrap/defaults".Mentoring instructions:
impl Step for Profileinsrc/bootstrap/setup.rsSubcommand::SetupinBuilder::newandBuilder::get_step_descriptions.cc @aswild - are you interested in tackling this after #96003?
@rustbot label +A-rustbuild +E-easy +E-medium
cc #96003 (comment)