Skip to content

bootstrap: correctly handle doc paths within submodules #135096

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jan 4, 2025

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Jan 4, 2025

Fixes #135041 by passing the correct submodule path when requiring submodules. This PR changes is_path_in_submodule to submodule_path_of. submodule_path_of returns the path of the containing submodule when given a path nested inside a submodule we handle, and None otherwise.

I tested this manually locally by unregistering the src/tools/cargo submodule, then running ./x doc src/tools/cargo/src/doc. This command fails on master with

thread 'main' panicked at src/bootstrap/src/utils/helpers.rs:441:5:
std::fs::read_dir(dir) failed with No such file or directory (os error 2)

since the require submodule fails as src/tools/cargo/src/doc is not a known submodule. Now we use the submodule path if such a nested-in-submodule-path is passed, and thus running this command with cargo submodule unregistered still succeeds:

Rustbook (x86_64-unknown-linux-gnu) - cargo
Doc path: /home/joe/repos/rust/build/x86_64-unknown-linux-gnu/doc/cargo/index.html
Build completed successfully in 0:00:11

r? @onur-ozkan

@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 Jan 4, 2025
@jieyouxu
Copy link
Member Author

jieyouxu commented Jan 4, 2025

Not super happy with how this macro still looks (it's still a bit convoluted), but at least the invocation-site is a bit nicer to use, I suppose.

There's also some pre-existing issues, such as:

  • We do not really sanity-check the doc path and submodule path, e.g. that they share prefix. That if the doc is inside a submodule, that the Step author correctly passed a path where (1) it's a path of a submodule that we handle and (2) it's the same submodule as where the doc resides in.
  • Not sure why requires_submodule silently fails? I think we just don't check it maybe?

@jieyouxu
Copy link
Member Author

jieyouxu commented Jan 4, 2025

@onur-ozkan do you think we should fix-forward (i.e. this PR) or revert #134967 then try to reland that?

@onur-ozkan
Copy link
Member

@onur-ozkan do you think we should fix-forward (i.e. this PR) or revert #134967 then try to reland that?

I think we can fix it right away. I need couple minutes to review this.

@onur-ozkan
Copy link
Member

@rustbot author

@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 Jan 4, 2025
@jieyouxu jieyouxu force-pushed the fix-doc-submodule-handling branch from 038a9f8 to 3e8ead1 Compare January 4, 2025 14:46
@jieyouxu
Copy link
Member Author

jieyouxu commented Jan 4, 2025

Changes since last review:

  • Dropped unrelated macro changes.
  • Rename is_path_in_submodule to submodule_path_of. submodule_path_of returns Some(submodule_path) if the given path is nested inside a known submodule or if the given path is already a submodule, and None otherwise.
  • Adjusted the helper's tests as suitable.

Tested this locally again by unregistering src/tools/cargo (git submodule deinit -f src/tools/cargo), and running ./x doc src/tools/cargo/src/doc.

@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 Jan 4, 2025
@jieyouxu jieyouxu force-pushed the fix-doc-submodule-handling branch from 3e8ead1 to 399e69e Compare January 4, 2025 14:53
@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu force-pushed the fix-doc-submodule-handling branch from 399e69e to 4406f42 Compare January 4, 2025 15:14
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to r=me once CI goes green

@jieyouxu
Copy link
Member Author

jieyouxu commented Jan 4, 2025

PR CI is 🍏
@bors r=@onur-ozkan rollup

@bors
Copy link
Collaborator

bors commented Jan 4, 2025

📌 Commit 4406f42 has been approved by onur-ozkan

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 Jan 4, 2025
@jieyouxu
Copy link
Member Author

jieyouxu commented Jan 4, 2025

@bors p=1 (contributor roadblock, feel free to include in rollup)

@jieyouxu
Copy link
Member Author

jieyouxu commented Jan 4, 2025

@bors p=6 (scheduling this before the next-in-line rollup)

@jieyouxu jieyouxu changed the title bootstrap: revamp book! macro and correctly handle doc paths within submodules bootstrap: correctly handle doc paths within submodules Jan 4, 2025
@bors
Copy link
Collaborator

bors commented Jan 4, 2025

⌛ Testing commit 4406f42 with merge ead4a8f...

@bors
Copy link
Collaborator

bors commented Jan 4, 2025

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing ead4a8f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 4, 2025
@bors bors merged commit ead4a8f into rust-lang:master Jan 4, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 4, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ead4a8f): 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 (secondary 0.8%)

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.8% [0.8%, 0.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results (secondary 2.8%)

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)
2.8% [2.4%, 3.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 762.575s -> 762.69s (0.02%)
Artifact size: 325.64 MiB -> 325.64 MiB (-0.00%)

@jieyouxu jieyouxu deleted the fix-doc-submodule-handling branch January 5, 2025 07:03
# for free to join this conversation on GitHub. Already have an account? # 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. 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.

thread 'main' panicked at src/bootstrap/src/utils/helpers.rs:441:5
6 participants