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.

Sign up for GitHub

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

Jump to bottom

get rid of duplicate primitive_docs #115494

Merged
merged 1 commit into from
Sep 18, 2023
Merged

get rid of duplicate primitive_docs #115494

merged 1 commit into from
Sep 18, 2023

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 3, 2023

Having this duplicate makes editing that file very annoying. And at least locally the generated docs still look perfectly fine...

@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2023

r? @joshtriplett

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

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 3, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2023

Cc #87073 @jyn514

@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2023

Ah, this seems to be the key point that wasn't mentioned in the files

this doesn't use purely relative links because that introduces new broken links when the docs are re-exported (e.g. String's &str deref impl, or Vec's slice deref impl).

But I feel like duplicating the file is not a great solution for that... it makes for a pretty bad contributor experience when editing that file.

@the8472
Copy link
Member

the8472 commented Sep 3, 2023

Symlinks would solve that but they cause issues for windows devs who don't have developer mode enabled.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 4, 2023

Hm, CI passed to maybe the quoted statement about broken links is not true any more...?

@RalfJung
Copy link
Member Author

RalfJung commented Sep 4, 2023

Symlinks would solve that but they cause issues for windows devs who don't have developer mode enabled.

Yeah I figured that was the reason for this. But my backup plan if we cannot include! the same file would be to make bootstrap copy the file from core to std before the build (and placing the std file in gitignore). That would still avoid having the file checked in twice.

@RalfJung RalfJung changed the title try to get rid of duplicate primitive_docs get rid of duplicate primitive_docs Sep 5, 2023
@RalfJung RalfJung mentioned this pull request Sep 6, 2023
Merged
@Mark-Simulacrum
Copy link
Member

We might be able to copy it somewhere else, but src directories are read-only mounted in CI. Should be workable otherwise...

@RalfJung
Copy link
Member Author

RalfJung commented Sep 6, 2023

Currently it doesn't look like we need to copy it at all?

@RalfJung
Copy link
Member Author

r? @Mark-Simulacrum
since you also reviewed #87073

@bors
Copy link
Contributor

bors commented Sep 16, 2023

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

@RalfJung
Copy link
Member Author

(I'm going to wait for feedback on the approach before rebasing; this will obviously have a conflict each time the file is changed.)

@Mark-Simulacrum
Copy link
Member

Ah, this seems to be the key point that wasn't mentioned in the files

this doesn't use purely relative links because that introduces new broken links when the docs are re-exported (e.g. String's &str deref impl, or Vec's slice deref impl).

But I feel like duplicating the file is not a great solution for that... it makes for a pretty bad contributor experience when editing that file.

I've been trying to reproduce this problem... linkcheck doesn't seem to find problems on this branch, and poking around a couple of primitive doc files I'm not seeing broken links either (but perhaps I'm not finding something). My inclination is to merge and we can back it out if problems arise.

@RalfJung did you manage to reproduce the problem above? If so, then the alternative approach of bootstrap copying files around might be reasonable (copying from core to a std-loaded directory would be my preference).

@RalfJung
Copy link
Member Author

Hm, maybe it works because neither str nor slice actually use any relative links right now? But I also don't think Deref creates a copy of the type docs so I'm rather confused by that comment. Maybe rustdoc behavior also changed.

@RalfJung
Copy link
Member Author

I've added a relative test link in str, let's see if CI complains.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 18, 2023 •
edited
Loading

CI seems fine. So I think we can conclude that something changed since back then and relative links are actually okay now?

I'll take the commit back out of the PR. If you want to check it out, it's ccbb7b3.

@Mark-Simulacrum
Copy link
Member

Yeah, maybe. Let's proceed for now and we can revert & re-evaluate if folks report problems. @bors r+

@bors
Copy link
Contributor

bors commented Sep 18, 2023

📌 Commit 7b7caae has been approved by Mark-Simulacrum

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 Sep 18, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 18, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2023
@bors
Auto merge of rust-lang#115940 - matthiaskrgr:rollup-5ps9ln1, r=matth…
4d69266 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#109409 (Add `minmax{,_by,_by_key}` functions to `core::cmp`)
 - rust-lang#115494 (get rid of duplicate primitive_docs)
 - rust-lang#115663 (ci: actions/checkout@v3 to actions/checkout@v4)
 - rust-lang#115762 (Explain revealing of opaque types in layout_of ParamEnv)
 - rust-lang#115891 (simplify inject_impl_of_structural_trait)
 - rust-lang#115932 (Expand infra-ci reviewer list)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2023
@bors
Auto merge of rust-lang#115940 - matthiaskrgr:rollup-5ps9ln1, r=matth…
65ea825 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#109409 (Add `minmax{,_by,_by_key}` functions to `core::cmp`)
 - rust-lang#115494 (get rid of duplicate primitive_docs)
 - rust-lang#115663 (ci: actions/checkout@v3 to actions/checkout@v4)
 - rust-lang#115762 (Explain revealing of opaque types in layout_of ParamEnv)
 - rust-lang#115891 (simplify inject_impl_of_structural_trait)
 - rust-lang#115932 (Expand infra-ci reviewer list)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0a66c87 into rust-lang:master Sep 18, 2023
@rustbot rustbot added this to the 1.74.0 milestone Sep 18, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2023
@rust-timer
Unrolled build for rust-lang#115494
66ccbec 
Rollup merge of rust-lang#115494 - RalfJung:primitive_docs, r=Mark-Simulacrum

get rid of duplicate primitive_docs

Having this duplicate makes editing that file very annoying. And at least locally the generated docs still look perfectly fine...
@RalfJung RalfJung deleted the primitive_docs branch September 19, 2023 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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) T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.
7 participants
@RalfJung @rustbot @rust-log-analyzer @the8472 @Mark-Simulacrum @bors @joshtriplett