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

Add x {check,build,doc} {compiler,library} aliases. #95504

Merged
merged 1 commit into from
Apr 24, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 30, 2022

While working on #95503, I realized that it will interfere with existing command lines:
Currently people run x build library/std expecting it to "add all library crates to the sysroot",
but after that change, it will only build libstd and its dependencies (and add them to the sysroot), not libtest or libproc_macro.

That will work for local testing in most cases, but could be confusing. Even if not, though, I think x build library is more clear about what actually happens than the current x build library/std.

The intended end goal is something like:

  • For check/build/doc, we have library + compiler aliases, which correspond to basically "most possible" for that piece. This is the intended path of entry (rather than library/test or similar as today) for when you just want the thing to work -- for example, getting a compiler that is "crates.io-compatible" would be roughly x.py build library). Add x {check,build,doc} {compiler,library} aliases. #95504
  • Specific crate invocations build up to that crate, which means that if you don't care about tests you probably want x.py build library/proc_macro or library/std for faster build times. bootstrap: Allow building individual crates #95503

Note that this is already implemented today for the doc command and seems to work pretty well in practice.

I plan to change the dev-guide and various instructions in the README to build library once this is merged.

@rustbot label +A-rustbuild

While working on rust-lang#95503,
I realized that this will interfere with existing command lines:
Currently people run `x build library/std` expecting it to be added to the sysroot,
but after that change, it will *only* build `libstd` without making it available
for the toolchain.

It's debatable whether that's a breaking change that will be accepted; if so, this PR is absolutely
necessary to make sure there's a command for "build the standard library and add it to the sysroot".
Even if not, though, I think `x build library` is more clear about what actually happens than the
current `x build library/std`.

For consistency, also add support for `compiler` and all other command variants.  Note that `doc
compiler` was already supported, so in a sense this is just fixing an existing inconsistency.

I plan to change the dev-guide and various instructions in the README to `build library` once this is merged.
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2022
@jyn514 jyn514 changed the title Add x {check,build,doc} {compiler/library} aliases. Add x {check,build,doc} {compiler,library} aliases. Mar 31, 2022
@mark-i-m
Copy link
Member

Would it make sense to add a x install blah command. I am actually surprised that build doesn't behave like cargo...

@jyn514
Copy link
Member Author

jyn514 commented Mar 31, 2022

@mark-i-m x install library/std already exists, I can add a library alias for it too if you like. Not sure how many people are using install, I've heard @Mark-Simulacrum say it's buggy.

@jyn514
Copy link
Member Author

jyn514 commented Mar 31, 2022

I am actually surprised that build doesn't behave like cargo...

What is different that surprises you?

@mark-i-m
Copy link
Member

Currently people run x build library/std expecting it to be added to the sysroo

@jyn514 This...

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Apr 10, 2022

It's not clear to me why we can't make crates available in the sysroot as they get built (effectively), rather than at the end of a given step. x.py build library/std making core, alloc, and std available in the sysroot but not building proc_macro and test seems fine to me, for example.

I'm not sure there's a rationale I can put my finger on yet but supporting the sub-workspace aliases (library, compiler) feels off to me (including where they're available today). I think the primary reason here is that conceptually there's no such concept at the Cargo level, whereas library/std maps to a Cargo understood concept (a crate). That feels to some extent like rationale in search of rationale rather than a good reason though, so it's something I want to think more on. It also dilutes the current mapping of "what paths can I use" to some extent, though that's maybe OK too.

It sounds like the expected motivation here is likely addressed by instead making library/std map to "std and into sysroot", which seems more logical and easily explainable to me, and I would hope isn't actually that hard. So maybe we can just close this PR?

Happy to hear objections or thoughts on any of the above.

@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2022
@jyn514
Copy link
Member Author

jyn514 commented Apr 10, 2022

It's not clear to me why we can't make crates available in the sysroot as they get built (effectively), rather than at the end of a given step. x.py build library/std making core, alloc, and std available in the sysroot but not building proc_macro and test seems fine to me, for example.

Ooh, this is an excellent idea! It solves my original problem completely and it also seems like the intuitive behavior :) I'll do that regardless of whether we add the new aliases or not.

I'm not sure there's a rationale I can put my finger on yet but supporting the sub-workspace aliases (library, compiler) feels off to me (including where they're available today).

Another main motivation I had was making commands more consistent. Right now, these work:

  • x doc compiler
  • x doc std
  • x build std
  • x check std

and these break:

  • x doc library
  • x build compiler
  • x build library
  • x check compiler
  • x check library

It looks like nothing uses library before this PR, which is maybe an indication I should be using std instead, but that feels even more arbitrary ...

I also find it strange that compiler works for doc but nothing else. It seems like we should support it for everything or for nothing. Note that "for nothing" would currently be a regression precisely because build/check use all_crates and doc uses crates, so e.g. doc compiler/rustc will only generate docs for a single crate. doc compiler/* currently fails because codegen_cranelift and codegen_gcc are not in the workspace:

thread 'main' panicked at 'no entry found for key', src/bootstrap/doc.rs:613:39

@rustbot ready

@rustbot 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 Apr 10, 2022
@Mark-Simulacrum
Copy link
Member

I also find it strange that compiler works for doc but nothing else. It seems like we should support it for everything or for nothing. Note that "for nothing" would currently be a regression precisely because build/check use all_crates and doc uses crates, so e.g. doc compiler/rustc will only generate docs for a single crate. doc compiler/* currently fails because codegen_cranelift and codegen_gcc are not in the workspace:

Yeah, this is confusing and unfortunate. I don't think where there are and aren't aliases for the 'directories' was picked intentionally; it may make sense to add library and compiler throughout as roots for those respective trees. At the same time, at least for compiler, it probably makes sense to actually attach that to "fully assemble" rather than just building (i.e., copy bin/rustc). I guess maybe that's implicit -- I forget how our assembling works precisely today.

I think the doc vs. check/build deviation is somewhat of a question of character -- for build and check, you don't really have any choice to build "up to" that crate (in the crate graph). For doc, you can build docs for any given crate (right?), you don't need the dependencies documented too.

I'm not well-versed enough in generating docs locally (I've very rarely done it) so it's hard for me to gauge whether the expectation there is met. My intuition is that most folks generating docs locally are doing so after editing them, not to view docs "in general", which implies that the by-crate behavior there is the correct one, and maybe a reasonable thing to deviate from build/check with.


I think my opinion is that I'm not really sure here and am willing to here a concrete proposal, but my current thinking is to do this:

  • For check/build, we have library + compiler aliases, which correspond to basically "most possible" for that piece. This is the intended path of entry (rather than library/test or similar as today) for when you just want the thing to work -- for example, getting a compiler that is "crates.io-compatible" would be roughly x.py build library).
    • Specific crate invocations build up to that crate, which means that if you don't care about tests you probably want x.py build library/proc_macro or library/std for faster build times.
  • For doc, by the rationale above, we have:
    • compiler corresponds to building all of the compiler crate docs
    • library corresponds to building all of the library crate docs
    • These are mostly probably not actually the thing to use, but are a decent approximation and so likely work OK. (Instead, most users probably only want to build docs for the particular crate they changed).
    • Specific crate invocations correspond to building just that crate's doc (regardless of library or compiler tree). This is I believe identical to today.

This unifies things for the most part and I think works out OK -- and corresponds(?) to the behavior you'd get from native Cargo with -p=foo arguments for each of build + check + doc, right? Not 100% sure there. That seems encouraging to me.

What do you think about this? Definitely not feeling extremely strong about it myself, very open to changing it.

@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2022
@jyn514
Copy link
Member Author

jyn514 commented Apr 21, 2022

For doc, you can build docs for any given crate (right?), you don't need the dependencies documented too.

This is true, yes. You still have to check the dependencies, but rustdoc can't emit metadata and so the only difference is whether it will generate relative links to the dependencies or not.

Specific crate invocations correspond to building just that crate's doc (regardless of library or compiler tree). This is I believe identical to today.

👍 yup, x doc core only documents core and not std.

$ x doc core --stage 0
Documenting stage0 std (x86_64-unknown-linux-gnu(x86_64-unknown-linux-gnu))
 Documenting core v0.0.0 (/home/jnelson/rust-lang/rust/library/core)
^C  Building [                             ] 0/1: core(doc)

and corresponds(?) to the behavior you'd get from native Cargo with -p=foo arguments for each of build + check + doc, right?

Yes, exactly! This is how #95503 works under the hood, it's just passing -p foo to cargo :) where foo is the crate name instead of the path. (I think there's some inconsistency currently about whether you can use crate names in x.py or whether you have to use the path, but that's a problem for another day.)

I like your suggestion to unify the behavior! I think that will be the end behavior after #95503 :) but I'll make sure to test in that PR.

  • For check/build, we have library + compiler aliases, which correspond to basically "most possible" for that piece. This is the intended path of entry (rather than library/test or similar as today) for when you just want the thing to work -- for example, getting a compiler that is "crates.io-compatible" would be roughly x.py build library).
    For doc, by the rationale above, we have:
  • compiler corresponds to building all of the compiler crate docs
  • library corresponds to building all of the library crate docs

That's what I've implemented here, I think. If you want, I can wait to merge it until #95503 lands - don't have a strong opinion on the ordering.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 21, 2022
@Mark-Simulacrum
Copy link
Member

I haven't looked at the code just now, but at least the PR description needs to be updated (probably easiest to copy my proposal comment - since it sounds like that's the end state). Particularly want to avoid the sysroot no-copy line since that'll be confusing looking back on it. Presuming code is good and implements (as you say) proposed unification, I see no reason not to merge this before the other PR that makes it more necessary.

@jyn514
Copy link
Member Author

jyn514 commented Apr 23, 2022

at least the PR description needs to be updated (probably easiest to copy my proposal comment - since it sounds like that's the end state). Particularly want to avoid the sysroot no-copy line since that'll be confusing looking back on it.

Thanks, done.

@jyn514
Copy link
Member Author

jyn514 commented Apr 23, 2022

Currently people run x build library/std expecting it to be added to the sysroo

@jyn514 This...

Replied to this on Zulip.

@Mark-Simulacrum
Copy link
Member

@bors r+

I think we can leave install to future work (and probably just not touch it).

@bors
Copy link
Contributor

bors commented Apr 24, 2022

📌 Commit 9f38ce0 has been approved by Mark-Simulacrum

@bors 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 Apr 24, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2022
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#94893 (diagnostics: regression test for `<usize as Iterator>::rev`)
 - rust-lang#95504 (Add `x {check,build,doc} {compiler,library}` aliases.)
 - rust-lang#96237 (compiletest: combine `--*-python` args)
 - rust-lang#96303 (Improve bootstrap tests)
 - rust-lang#96352 (Improve span for `consider adding an explicit lifetime bound` suggestions under NLL)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4724040 into rust-lang:master Apr 24, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 24, 2022
@jyn514 jyn514 deleted the library-alias branch July 11, 2022 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants