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

Change --extern-html-root-url to use crate source path as key #78909

Closed
wants to merge 4 commits into from

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Nov 9, 2020

This is the most correct solution I see to #76296, this allows setting the html-root-url for all crates in the dependency tree, no matter how many duplicates of a specific crate name exist.

I have prototyped the associated docs.rs changes in my script that builds in the same fashion as it: Nemo157/dotfiles@d452627. Before this is merged we would want to do the same for docs.rs itself (I'll make a PR for this shortly), it's forwards-compatible to start emitting both sets of flags and remove the current ones later, extra unused flags are just ignored.

fixes #76296

As well as testing the ability to override the url, run the same
testcase with the `doc(html_root_url)` specified url.
@rust-highfive
Copy link
Collaborator

r? @jyn514

(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 Nov 9, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 9, 2020

This seems to expose build details a little bit - does cargo even provide a way to get the .so file that corresponds to a crate? I wouldn't want to make this breaking change without knowing there's a way to actually use it, if you could put together a demo of a cargo-based project that would make me feel a lot better.

cc @eddyb - this changes rustdoc to look up info about a crate by the source path (extern-html-root-url/auxiliary/libextern_html_root_url.so) instead of the crate name (extern_html_root_url) so that rustdoc can distinguish between versions of a crate with the same name.

@jyn514 jyn514 added A-metadata Area: Crate metadata T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 9, 2020
@Nemo157
Copy link
Member Author

Nemo157 commented Nov 9, 2020

This seems to expose build details a little bit - does cargo even provide a way to get the .so file that corresponds to a crate? I wouldn't want to make this breaking change without knowing there's a way to actually use it, if you could put together a demo of a cargo-based project that would make me feel a lot better.

See the link to my cargo doc-like-docs.rs changes, using cargo check --message-format=json it emits compiler-artifact messages with the files that were produced for each crate.

@Nemo157
Copy link
Member Author

Nemo157 commented Nov 10, 2020

It needs significant cleanup, but rust-lang/docs.rs@master...Nemo157:issue-76296 mostly implements this on the docs.rs side.

example command from docs.rs build log

image

@ehuss
Copy link
Contributor

ehuss commented Nov 10, 2020

Hm, using extern paths is an interesting approach! It does neatly solve the problem of having some unique identifier to correlate each extern crate. Another approach I had thought about was to encode the -C extra-filename in the crate metadata, and then use that hash as an identifier, but that didn't seem very good.

It would be nice to have some strategy to also fix #56169. I'm not sure how this could fit in with that, or if that will need to be solved with a completely different approach.

I would be concerned about comparing paths directly. Using absolute paths tends to be tricky and runs the risk of not working in various situations. same-file would be an option to reliably compare paths, but I'm uncertain if that would have a significant performance impact.

Regarding the concern about long command-line paths, I'm thinking that the best option is to use a response file. I'm a little uncomfortable doing that in Cargo (it tends to be a pain to work with), but I'm not sure if there are many other options.

Even though I am the author of the crate_extern_paths query, I am uncertain if it is appropriate to use it in this fashion.

The documentation should probably be updated.

@jyn514
Copy link
Member

jyn514 commented Nov 11, 2020

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned jyn514 Nov 11, 2020
@Nemo157
Copy link
Member Author

Nemo157 commented Nov 11, 2020

One thought I just had to reduce the command size back a bit; we probably don't need the absolute path, given the metadata suffix just using the filename would probably be enough, so that would only add an extra ~26 characters per crate.

@bors
Copy link
Contributor

bors commented Nov 17, 2020

☔ The latest upstream changes (presumably #79104) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@crlf0710
Copy link
Member

crlf0710 commented Dec 4, 2020

Triage: There's merge conflicts now.

@crlf0710 crlf0710 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 Dec 4, 2020
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2021
@JohnCSimon
Copy link
Member

Ping from triage
@Nemo157 - can you please resolve the merge conflict, and alert the reviewer? Thank you!

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2021
@Dylan-DPC-zz
Copy link

@Nemo157 any updates on this?

@crlf0710 crlf0710 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 5, 2021
@crlf0710 crlf0710 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 5, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2021
@crlf0710 crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 9, 2021
@Dylan-DPC-zz
Copy link

Closing this as inactive

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-metadata Area: Crate metadata S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--extern-html-root-url uses library names to identify dependencies, failing to handle multiple versions
8 participants