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

Merged

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 (CLI) 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

@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 May 29, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 29, 2024
…map-path-prefix, r=GuillaumeGomez

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

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`
bors added a commit to rust-lang-ci/rust that referenced this pull request May 29, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#107099 (rustdoc: Add support for --remap-path-prefix)
 - rust-lang#125693 (Format all source files in `tests/coverage/`)
 - rust-lang#125700 (coverage: Avoid overflow when the MC/DC condition limit is exceeded)
 - rust-lang#125705 (Reintroduce name resolution check for trying to access locals from an inline const)
 - rust-lang#125708 (tier 3 target policy: clarify the point about producing assembly)
 - rust-lang#125715 (remove unneeded extern crate in rmake test)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r-
#125725 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 29, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 7, 2024
@bors
Copy link
Contributor

bors commented Jun 7, 2024

☔ The latest upstream changes (presumably #125798) 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 d66718c to d9f78cb Compare June 9, 2024 17:35
@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2024

Some changes occurred in run-make tests.

cc @jieyouxu

@edward-shen
Copy link
Contributor Author

edward-shen commented Jun 9, 2024

Changes:

  • Rebased on top of the submodule re-org
  • Looks like the Windows failure in the rollup is because they use "exit code" instead of "exit status". I chose to normalize the wording code/status in the unit test to fix this, as panicking in the unit test also reveals a remapped location to test for.

@edward-shen
Copy link
Contributor Author

Oops, @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 Jun 9, 2024
@GuillaumeGomez
Copy link
Member

Let's try again!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 9, 2024

📌 Commit d9f78cb has been approved by GuillaumeGomez

It is now in the queue for this repository.

@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 Jun 9, 2024
@bors
Copy link
Contributor

bors commented Jun 10, 2024

⌛ Testing commit d9f78cb with merge 6d94a87...

@bors
Copy link
Contributor

bors commented Jun 10, 2024

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 6d94a87 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 10, 2024
@bors bors merged commit 6d94a87 into rust-lang:master Jun 10, 2024
7 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 10, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6d94a87): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.3% [2.3%, 2.3%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.3% [2.3%, 2.3%] 1

Cycles

Results (secondary -2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: missing data
Artifact size: 319.16 MiB -> 319.73 MiB (0.18%)

@edward-shen edward-shen deleted the edward-shen/rustdoc-remap-path-prefix branch June 10, 2024 05:06
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 (CLI) to the compiler disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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. 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.