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

Revert behaviour change for bootstrap doc command #113123

Conversation

GuillaumeGomez
Copy link
Member

Fixes #113101.

In #111955 and more precisely in 58e18dd, the behaviour for x.py doc std changed: before it was generating docs for core and alloc alongside std, after it was only generating docs for std.

r? @jyn514

@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) labels Jun 28, 2023
@rust-log-analyzer

This comment has been minimized.

…ocumentation for the crate and its deps again
@GuillaumeGomez
Copy link
Member Author

And of course I forgot to run fmt. Fixed. ^^'

@jyn514
Copy link
Member

jyn514 commented Jun 28, 2023

i don't think we should do this. std refers to a single crate. use library if you want to document all crates.

@GuillaumeGomez
Copy link
Member Author

I don't want all crates, I want std and its dependencies.

@jyn514
Copy link
Member

jyn514 commented Jun 28, 2023

ok. for that you can use x doc core alloc std.

@jyn514
Copy link
Member

jyn514 commented Jun 28, 2023

by the way, the previous behavior was to document all library crates, so reverting the PR still won't get you what you want.

@GuillaumeGomez
Copy link
Member Author

ok. for that you can use x doc core alloc std.

That's much longer than it used to.

by the way, the previous behavior was to document all library crates, so reverting the PR still won't get you what you want.

No, the previous behaviour is exactly the same as the one I put here: I enter a crate name and it stops documenting crates when it finds this name. Which is why the order of names in the constant is important (as mentioned in the comment). I wrote it this way intentionally and I'd like to put back this behaviour if possible.

It's not the first time the behaviour of bootstrap changes and we discover it afterwards. It'd be nice that the impacted teams are notified of this so they understand why the change is needed and what's the new syntax. In this case, I don't understand the logic behind it except for adding library, which doesn't do what I want. In the current case, took me a while to understand what I thought was a bug to actually be a change I wasn't aware of in bootstrap.

@ChrisDenton
Copy link
Member

So if this is reverted then I'm wondering how I would document only a single crate?

@GuillaumeGomez
Copy link
Member Author

You can't with this reversal. Why do you want only one crate btw? What's your use case?

@ChrisDenton
Copy link
Member

It's usually how I check doc changes to std now. It'd be kinda cool if I could only preview the specific page(s) I'm interested in but failing that the whole crate is good enough.

@GuillaumeGomez
Copy link
Member Author

It adds around 3 extra seconds on my computer for documenting core and alloc. If it's an issue for you, we can add an extra option to bootstrap maybe so you can still only document one crate like x.py doc std --no-deps?

@jyn514
Copy link
Member

jyn514 commented Jun 28, 2023

I'm sorry I didn't tag you on the original PR. This change affects more than just the rustdoc team, though (as Chris shows).

I don't want to make the interface for doc different than the interface for all the other bootstrap commands. Right now there's an unambiguous meaning for std and it is the same between check, build, and doc. Changing doc to use --no-deps means that it's inconsistent with the other commands.

@jyn514
Copy link
Member

jyn514 commented Jun 28, 2023

I don't want all crates, I want std and its dependencies.

on my laptop, x doc std (with your PR) takes 15 seconds and x doc library also takes 15 seconds. So I'm not sure I see much downside to using library instead. If you don't like that it's longer than std we can add a libs alias.

@GuillaumeGomez
Copy link
Member Author

For coherency sake, it might be better indeed... For me it takes 1 extra second to build the other two libs, so not an issue either. The only problems remaining are:

  • changing a habit: I can write an alias, it's just an annoyance.
  • rustdoc search testing

You also mentioned the use case from Chris, but it seems to be much more specific and less needed than the rustdoc team needs. Well in any case, I'm super annoyed that this behaviour I wrote on purpose was removed. If you don't plan to accept this revert, please just close the PR.

@oli-obk oli-obk closed this Jun 28, 2023
@GuillaumeGomez GuillaumeGomez deleted the revert-bootstrap-doc-behaviour branch June 28, 2023 18:47
@oli-obk
Copy link
Contributor

oli-obk commented Jun 28, 2023

Seems like we found sth that works for everyone. The takeaway: if in doubt, ping. unsubscribing is easy.

@GuillaumeGomez
Copy link
Member Author

I didn't understand your comment. In any case, the current state is not great because it changes the behaviour of a command used by the rustdoc and the alternative is not doing exactly the same. Not great, we'll see if problems come from that, although I doubt it.

@Manishearth
Copy link
Member

Worth pointing out: bootstrap is extremely confusing and inconsistent. Any attempt to fix that will lead to changes in current behavior that people may have formed habits around. It's worth trying to notice those changes and keep people informed, but that can only be best effort.

In general changes in habit are a temporary thing and the benefits of consistency far outweigh the annoyances, so as such changes happen in the future I do think the right decision is to lean towards self-consistency rather than attempting to keep old behavior and workflows. Workflows can be changed, if something that was previously possible and necessary is no longer possible it's worth properly interrogating and perhaps designing something for that use case from first principles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

source links in std are not relative
7 participants