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

Fix crate-version with rustdoc in bootstrap. #75539

Merged
merged 2 commits into from
Aug 15, 2020

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Aug 14, 2020

Cargo will now automatically use the --crate-version flag (see rust-lang/cargo#8509). Cargo has special handling to avoid passing the flag if it is passed in via RUSTDOCFLAGS, but the rustdoc wrapper circumvents that check. This causes a problem because rustdoc will fail if the flag is passed in twice. Fix this by using RUSTDOCFLAGS.

This will be necessary when 1.47 is promoted to beta, but should be safe to do now.

@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 Aug 14, 2020
@ehuss
Copy link
Contributor Author

ehuss commented Aug 14, 2020

Hm, so I completely forgot that the non-dev version string has a space in it, which won't work with RUSTDOCFLAGS. That's...unfortunate. I'm trying to decide how to deal with this. Some options I'm considering:

  • Use cargo's -Zadvanved-env which allows spaces in env vars via TOML. It only works with config values, so it would need to be specified via CARGO_BUILD_RUSTDOCFLAGS.
    • I could introduce space support for RUSTDOCFLAGS in cargo, but that would need to wait until beta promotion.
  • Use cargo's --config CLI option which allows passing config values on the cli via TOML (like build.rustdocflags).
  • Revert Stabilize -Z crate-versions cargo#8509 until there is better support for args with spaces.
  • Maybe pass --crate-version as a command-line flag here. I don't like that because the rustc/rustdoc flags use cargo doc which doesn't allow flags.

Do any of those options sound reasonable? Or any other ideas? I would probably lean towards advanced-env.

@Mark-Simulacrum
Copy link
Member

The version string is 1.47.0-nightly (81dc88f 2020-08-13), right? Could we swap the space for   or perhaps some Unicode space?

An other option is to pass a fake version via RUSTDOCFLAGS and then swap it out in our wrapper...

Do you have some sense of how stable we expect the toml environment stuff to be? If it's pretty likely to not change (much) then I think that's probably our best bet. I wouldn't mind the (simpler?) fix of not using a real space though.

@ehuss
Copy link
Contributor Author

ehuss commented Aug 15, 2020

The version string is 1.47.0-nightly (81dc88f 2020-08-13), right?

Yes.

Could we swap the space for   or perhaps some Unicode space?

I had the same idea. Rustdoc escapes the &, so that won't work. Unicode spaces work, though I was worried that was a little too magical or potentially confusing. I'm pretty sure it won't break on linux, macos, and windows, but I'm not sure about other unusual platforms.

With nbsp, it gets a little too wide for the sidebar, particularly with the nightly/beta strings. Of course Unicode has a million other spaces to choose from. en-space (\u2002) looks pretty much the same as the original. I went ahead and pushed a commit with that change, WDYT?

Do you have some sense of how stable we expect the toml environment stuff to be?

Unfortunately I don't. It was an experiment that we haven't really pursued too much.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me either way - I think a new line might be a bit better, but I don't care much either way.

// does not support arguments with regular spaces. Hopefully someday
// Cargo will have space support.
let rust_version = self.rust_version().replace(' ', "\u{2002}");
rustdocflags.arg("--crate-version").arg(&rust_version);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a bit out there, but should work more uniformly across platforms I think. If cargo is splitting by regular ASCII space only, we could use a new line here.

That said, thinking some more about this it seems really error prone - anyone can replace that splitting with split_whitespace which would break us here. But I feel like that's not too big a deal, we can fix it as needed. Would you be up for adding a Cargo test as well so that the (potential) breakage is detected earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I like newline better.

I'll put it on my todo list to add a regression test in cargo.

@ehuss ehuss force-pushed the fix-crate-version-rustdoc-bootstrap branch from 2593736 to 85a9cfa Compare August 15, 2020 03:16
@ehuss
Copy link
Contributor Author

ehuss commented Aug 15, 2020

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Aug 15, 2020

📌 Commit 85a9cfa 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 Aug 15, 2020
@bors
Copy link
Contributor

bors commented Aug 15, 2020

⌛ Testing commit 85a9cfa with merge 5205b97...

@bors
Copy link
Contributor

bors commented Aug 15, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 5205b97 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 15, 2020
@bors bors merged commit 5205b97 into rust-lang:master Aug 15, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants