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

Fix intra-doc links on docs.rs #1205

Merged
merged 5 commits into from
Jul 28, 2022
Merged

Fix intra-doc links on docs.rs #1205

merged 5 commits into from
Jul 28, 2022

Conversation

jplatte
Copy link
Member

@jplatte jplatte commented Jul 28, 2022

Finally, with the help of @Nemo157, I was able to reproduce #1180 locally by adding resolver = "2" to the root workspace config. I added an extra feature that enables all of tower and tower-https features, and is picked up by docs.rs through all-features = true, which ensures all the modules we want to link to are actually there when cargo doc is run.

Fixed #1180
Closes #1184

@jplatte jplatte requested a review from davidpdrsn July 28, 2022 12:32
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Fantastic! 🎉

I cannot test it but I take your word for it 😅 I'll ship a new version end of next week.

@davidpdrsn
Copy link
Member

The CI failure seems unrelated but not sure 🤔

@jplatte
Copy link
Member Author

jplatte commented Jul 28, 2022

CI failure is likely caused by the first commit. I'll look into it when I get the time.

jplatte added 2 commits July 28, 2022 17:11
Tungstenite 0.17.1 has a higher MSRV, and there should be no reason to
use it over 0.17.2.
@jplatte jplatte force-pushed the jplatte/intra-doc-links branch from 0225eba to 1d0800b Compare July 28, 2022 16:13
@jplatte jplatte requested a review from davidpdrsn July 28, 2022 16:14
… to avoid dependencies with a broken minimum-versions chain.
@jplatte jplatte merged commit f0f6068 into main Jul 28, 2022
@jplatte jplatte deleted the jplatte/intra-doc-links branch July 28, 2022 17:14
@lameferret
Copy link

So to reiterate, the issue was:

tower-http module intra doc link didn't work in docs.rs because the module that were being linked were behind feature flag.

And doc.rs public build was using resolver=2 which strictly needs used features to be specified which tower-http didn't, while tower crates don't have such flags, therefore intra-doc link worked fine for it.

@Nemo157
Copy link

Nemo157 commented Jul 28, 2022

Close. The tower crates flags don't matter here, the resolver is only set by the root crate being built. The axum crate has set edition = "2021" which implies a default of resolver = "2", but locally it's within a workspace which overrides the resolver chosen, and that workspace used the default resolver = "1". So when you build in the repository the dev-dependencies features leak into the normal doc build and activate extra features, when you extract just the axum crate out of the repository and build it on its own those features don't leak.

@jplatte
Copy link
Member Author

jplatte commented Jul 28, 2022

Actually tower and tower-http both mattered, in the same way. Most links affected (at least in the doc section where I noticed) were for tower-http, but one was for tower (I think for a timeout middleware).

jplatte added a commit that referenced this pull request Oct 19, 2022
* Use version 2 of Cargo's feature resolver

* Increase minimum version of tungstenite

Tungstenite 0.17.1 has a higher MSRV, and there should be no reason to
use it over 0.17.2.

* Clean up and fix MSRV CI job

* Fix some intra-doc links not resolving correctly on docs.rs

* Bump minimum version of tower

… to avoid dependencies with a broken minimum-versions chain.
davidpdrsn pushed a commit that referenced this pull request Oct 20, 2022
* Use version 2 of Cargo's feature resolver

* Increase minimum version of tungstenite

Tungstenite 0.17.1 has a higher MSRV, and there should be no reason to
use it over 0.17.2.

* Clean up and fix MSRV CI job

* Fix some intra-doc links not resolving correctly on docs.rs

* Bump minimum version of tower

… to avoid dependencies with a broken minimum-versions chain.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Commonly used middleware" links are broken in middleware module docs
4 participants