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

rustdoc: Add support for --remap-path-prefix #107099

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edward-shen
Copy link
Contributor

@edward-shen edward-shen commented Jan 20, 2023

Adds --remap-path-prefix as an unstable option. This is implemented to mimic the behavior of rustc's --remap-path-prefix.

This flag similarly takes in two paths, a prefix to replace and a replacement string.

This is useful for build tools (e.g. Buck) other than cargo that can run doc tests.

cc: @dtolnay

@rustbot
Copy link
Collaborator

rustbot commented Jan 20, 2023

r? @GuillaumeGomez

(rustbot has picked a reviewer for you, use r? to override)

@rust-log-analyzer

This comment has been minimized.

@edward-shen edward-shen force-pushed the edward-shen/rustdoc-remap-path-prefix branch from f42071a to 986d0f4 Compare January 20, 2023 17:34
@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 20, 2023
@edward-shen edward-shen force-pushed the edward-shen/rustdoc-remap-path-prefix branch from 986d0f4 to a65e866 Compare January 20, 2023 17:40
@rust-log-analyzer

This comment has been minimized.

@edward-shen edward-shen force-pushed the edward-shen/rustdoc-remap-path-prefix branch 2 times, most recently from 6271d23 to 76e5d16 Compare January 20, 2023 20:29
@edward-shen
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2023
@GuillaumeGomez
Copy link
Member

The code looks good to me, but it comes a bit out of the blue so needs the rustdoc team to agree on it (very likely through FCP).

But first, what do you think @rust-lang/rustdoc ?

@edward-shen
Copy link
Contributor Author

but it comes a bit out of the blue so needs the rustdoc team to agree on it

My bad! I didn't think this would be a substantial change since rustc already had this flag. For future reference, should I have dropped in the zulip before the implementation?

@edward-shen edward-shen force-pushed the edward-shen/rustdoc-remap-path-prefix branch from 76e5d16 to 4bf4e60 Compare January 24, 2023 19:01
@GuillaumeGomez
Copy link
Member

At least opening an issue to explain your needs so it makes it easier for everyone to reach a solution. If this PR ends up being rejected, you'd have worked for nothing. :-/

@edward-shen
Copy link
Contributor Author

Changes:

  • rebased
  • Realized I made a function pub for a previous implementation attempt. It no longer needed to be pub so I removed it
  • Fixed copy-paste error in ui test.

@edward-shen
Copy link
Contributor Author

At least opening an issue to explain your needs so it makes it easier for everyone to reach a solution. If this PR ends up being rejected, you'd have worked for nothing. :-/

Got it--I'll be sure to do so in the future. Sorry again, and thank you for the gentle explanation!

@dtolnay
Copy link
Member

dtolnay commented Jan 24, 2023

@GuillaumeGomez As far as I can tell from the reference to Buck (which I am familiar with) the point of --remap-path-prefix for rustdoc is to get diagnostics inside of doctests to refer to the source directory tree of the build, not the build system's directory tree. For example without --remap-path-prefix:

---- buck-out/v2/gen/fbcode/882bd00a4ffb1e1d/tracing/stats/__tracing_stats__/__srcs/src/lib.rs - StatsLayer (line 70) stdout ----
error[E0433]: failed to resolve: use of undeclared type `StatsLayer`
 --> buck-out/v2/gen/fbcode/882bd00a4ffb1e1d/tracing/stats/__tracing_stats__/__srcs/src/lib.rs:71:13
  |
3 | let stats = StatsLayer::new("myservice", 8, false)
  |             ^^^^^^^^^^ not found in this scope
  |
help: consider importing this struct
  |
2 | use tracing_stats::StatsLayer;
  |

The build system would want to use --remap-path-prefix buck-out/v2/gen/fbcode/882bd00a4ffb1e1d/tracing/stats/__tracing_stats__/__srcs/=tracing/stats/ to get:

---- tracing/stats/src/lib.rs - StatsLayer (line 70) stdout ----
error[E0433]: failed to resolve: use of undeclared type `StatsLayer`
 --> tracing/stats/src/lib.rs:71:13
  |
3 | ...

It's a common technique for mature build systems to separate the files that get fed to build steps apart from the files that get touched by the programmers. There are many reasons for this; not all of them apply to every build system that uses this technique.

  • Determinism, no race conditions: if a user, or IDE, is mutating the source files during the time that a build is already running, the build system can still be 100% sure exactly which iteration of the input files the output of the build was performed against, and the exact delta to the subsequent build. It's not subject to a race condition between when the compiler ends up reading a file vs when the editor writes the file, which would be outside the build system's ability to inspect (leaving aside some kind of kernel module or sophisticated filesystem or syscall interceptor).

  • Hermeticity: the build system can sandbox build steps to keep them from reaching into arbitrary filesystem state that is not tracked by the build system's understanding of the precise inputs of every step.

  • Integration of code generators: for source files which are generated by code generators during the course of the build (such as protoc or thriftc or bindgen), the build system can map them to easily understood siblings of the crate root, for example using Buck's mapped_srcs and genrule. This is a lot nicer than Cargo's include!(concat!(env!("OUT_DIR" …)) thing.

  • Remote execution: build steps in a large build would typically not run on the same machine as the source files are being written on. In general the filesystem structure on the remote build environment are not identical to the filesystem paths on the developer's machine, but the build system is responsible for mapping between them.

  • Distributed caching: sharing build step output artifacts (including diagnostics/warnings, not just rlibs/rmeta/binaries) across different devs, or with a cache which is kept perpetually warm by CI. Again different machines could have different path structure (even different OS, as long as at least 1 is cross-compiling).

Edward or I would be happy to elaborate on any of the above if needed.

@edward-shen edward-shen force-pushed the edward-shen/rustdoc-remap-path-prefix branch from 4bf4e60 to b2d37cf Compare January 25, 2023 04:31
@bors
Copy link
Contributor

bors commented Feb 16, 2023

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

@dtolnay dtolnay removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 19, 2023
@workingjubilee workingjubilee added the A-cli Area: Command line interface to the compiler. label Mar 5, 2023
@KittyBorgX
Copy link
Member

@rustbot author
@edward-shen Hey! It looks like there are some merge conflicts in your PR. Can you sort them out?
Once you're done type in @rustbot review here so that the label can be changed to S-waiting-on-review and your PR should be good for a review!

@rustbot rustbot 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 29, 2023
@edward-shen edward-shen force-pushed the edward-shen/rustdoc-remap-path-prefix branch from b2d37cf to 51c1b4f Compare May 2, 2023 02:56
@edward-shen
Copy link
Contributor Author

Hey @KittyBorgX, thanks for the ping. Work has been busy, so the conflicts message slipped through the cracks.

Rebased and fixed merge conflicts.

@rustbot review

@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 May 2, 2023
@GuillaumeGomez
Copy link
Member

Looks good to me, thanks! Let's start an FCP then.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented May 15, 2023

Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 15, 2023
@Nemo157
Copy link
Member

Nemo157 commented May 15, 2023

What are the "minor adjustments"? Will they in some way affect applying the RFC 3127 changes to rustdoc too?

@notriddle
Copy link
Contributor

@Nemo157

The main place that source paths get embedded is the /src tree (#98220 #18370).

@edward-shen
Copy link
Contributor Author

What are the "minor adjustments"? Will they in some way affect applying the RFC 3127 changes to rustdoc too?

I think the minor adjustments phrase is a misnomer. A previous implementation of this required different args to pass to rustdoc to match what rustc would do, but that was changed so that no longer applies.

This change can be accurately described as "rustdoc now passes in --remap-path-prefix args to rustc_session, and now prefers remapped paths if they exist over preferring local paths".

This shouldn't affect RFC 3127. If the implementation for that RFC is done in rust_session, we should be able to pick up the changes for free. My understanding is that we just need to expose similar options for rustdoc and thread those args to the implementation as well.

@bors
Copy link
Contributor

bors commented Aug 16, 2023

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

@edward-shen edward-shen force-pushed the edward-shen/rustdoc-remap-path-prefix branch from 51c1b4f to dc42b59 Compare August 17, 2023 05:39
@rust-log-analyzer

This comment has been minimized.

@edward-shen edward-shen force-pushed the edward-shen/rustdoc-remap-path-prefix branch from dc42b59 to 00b1171 Compare August 17, 2023 05:55
@bors
Copy link
Contributor

bors commented Nov 16, 2023

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

@Dylan-DPC Dylan-DPC 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 Mar 14, 2024
@Dylan-DPC
Copy link
Member

@edward-shen if you can resolve these conflicts we can push this forward for a review. Thanks

@edward-shen edward-shen force-pushed the edward-shen/rustdoc-remap-path-prefix branch from 00b1171 to eb5a731 Compare March 15, 2024 00:50
@edward-shen
Copy link
Contributor Author

@Dylan-DPC, thanks for taking another look at this. I've rebased my changes and ran ./x t, so hopefully things should still be correct.

@edward-shen
Copy link
Contributor Author

@rustbot review

@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 Mar 15, 2024
@bors
Copy link
Contributor

bors commented Mar 15, 2024

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

Adds --remap-path-prefix as an unstable option. This is implemented to
mimic the behavior of rustc's --remap-path-prefix but with minor
adjustments.

This flag similarly takes in two paths, a prefix to replace and a
replacement string.
@edward-shen edward-shen force-pushed the edward-shen/rustdoc-remap-path-prefix branch from eb5a731 to d66718c Compare March 16, 2024 05:07
@GuillaumeGomez
Copy link
Member

I added it in the next rustdoc meetup agenda to be discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command line interface to the compiler. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

None yet