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

Justification of html_root_url is missing nuance #229

Closed
jyn514 opened this issue Nov 18, 2020 · 24 comments
Closed

Justification of html_root_url is missing nuance #229

jyn514 opened this issue Nov 18, 2020 · 24 comments
Labels
amendment Amendments to existing guidelines 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. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. T-libs Relevant to the libraries subteam, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Nov 18, 2020

cc #75, @dtolnay

From rust-cli/env_logger#185:

The scenario where this goes wrong (assuming you're the author of env_logger and not using html_root_url) is:

  1. Another downstream crate uses env_logger as a dependency
  2. downstream re-exports or in some other way links to env_logger
  3. A user or developer of downstream builds documentation locally with cargo doc --no-deps (without --extern-html-root-url, because in practice no one but docs.rs does that).

Then the links to env_logger will be broken. But there's a simple fix and the fix is to remove --no-deps. Is this really so common that it's worth recommending that library authors use html_root_url?

Note that I'm hoping to fix this properly in cargo instead: rust-lang/cargo#8296

@jyn514
Copy link
Member Author

jyn514 commented Nov 18, 2020

Note that this has nothing to do with docs.rs, docs.rs passes --extern-html-root-url which always overrides any html_root_url you declare yourself.

@BurntSushi
Copy link
Member

I'm pretty sure this guideline was created in a time where not specifying html_root_url was more problematic than it is today. But I can't remember the details to be honest. I don't think I use it in any of my crates and I've never heard any complaints.

@jyn514
Copy link
Member Author

jyn514 commented Nov 18, 2020

Oh hmm - I think when it was originally written rustdoc didn't support --extern-html-root-url, so docs.rs did need it. But that's not the case any more.

Should I make a PR removing the recommendation?

@jyn514
Copy link
Member Author

jyn514 commented Nov 18, 2020

Oh heh - it has a comment pointing to rust-lang/rust#42301, which I closed as wontfix a few days ago.

@BurntSushi
Copy link
Member

Oh hmm - I think when it was originally written rustdoc didn't support --extern-html-root-url, so docs.rs did need it. But that's not the case any more.

Should I make a PR removing the recommendation?

I'm not even sure docs.rs was even around back then. But I can't remember. :)

@jhpratt
Copy link
Member

jhpratt commented Nov 27, 2020

For what it's worth I'm fairly certain most crates (including all of mine) don't use this attribute, and I have never had an issue with links not working exactly as expected.

nnethercote added a commit to nnethercote/dhat-rs that referenced this issue Dec 8, 2020
@KodrAus KodrAus added the amendment Amendments to existing guidelines label Dec 21, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Dec 21, 2020

Sounds like we should remove the recommendation then! It's a bit of a footgun that we don't need anymore.

@KodrAus KodrAus added accepted An amendment that's been accepted and can be applied and removed accepted An amendment that's been accepted and can be applied labels Dec 21, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Dec 21, 2020

Actually, since we have just been talking about the way we make decisions on this repo I should try actually follow what we set out and kick off an FCP!

@KodrAus KodrAus added the T-libs Relevant to the libraries subteam, which will review and decide on the PR/issue. label Dec 21, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Dec 21, 2020

@rfcbot fcp merge

I propose we remove the guideline recommending html_root_url as it's no longer necessary.

@KodrAus
Copy link
Contributor

KodrAus commented Dec 21, 2020

Guess I'd better go find out why @rfcbot doesn't want to hang out here

@KodrAus KodrAus added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Dec 21, 2020
@rfcbot
Copy link

rfcbot commented Dec 21, 2020

Team member @KodrAus 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.

@jyn514
Copy link
Member Author

jyn514 commented Dec 21, 2020

👀 @rfcbot unchecked @BurntSushi 's checkbox

@KodrAus KodrAus added the I-nominated Indicates that an issue has been nominated for discussion during a team meeting. label Dec 21, 2020
@BurntSushi
Copy link
Member

Weird. I just checked it again.

@KodrAus KodrAus added disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. and removed needs-fcp This change is insta-stable, so needs a completed FCP to proceed. I-nominated Indicates that an issue has been nominated for discussion during a team meeting. labels Dec 22, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Dec 22, 2020

Oh wow... It did work 😮

@KodrAus
Copy link
Contributor

KodrAus commented Dec 22, 2020

Also note there was some lengthy discussion in #230 about this and its implications for non-Cargo build systems.

@KodrAus KodrAus removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Dec 22, 2020
@Kage-Yami
Copy link

I'll agree it makes sense to reduce the effort involved in making a "good" library, but is there some alternative that keeps these links (even to local documentation of dependencies, if need be), without cluttering the left-hand side?

Yes, -Z rustdoc-map: rust-lang/cargo#8296

Given I work solely in Stable, effectively no... but I've watched that issue now (I missed the reference in the OP before).

@rfcbot
Copy link

rfcbot commented Dec 31, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @KodrAus, I wasn't able to add the final-comment-period label, please do so.

@rfcbot
Copy link

rfcbot commented Jan 10, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

psst @KodrAus, I wasn't able to add the finished-final-comment-period label, please do so.

@KodrAus KodrAus added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Jan 12, 2021
Kage-Yami added a commit to Kage-Yami/dynamic-dns-client-for-cloudflare that referenced this issue Jan 20, 2021
rust-lang/api-guidelines#229 - while the issue is still open, the RFC has passed so will be merged in at some point.
@mgeisler
Copy link
Contributor

I also frequently build with --no-deps, but at the same time tend to forget to update the html_root_url when cutting new releases so I've started removing them.

As a side-note: that is why I created the version-sync crate. It makes it trivial to add an integration test which will fail if you forget to update the html_root_url (or any other piece of text which mentions the version number).

tesuji added a commit to tesuji/fburl-rs that referenced this issue Feb 21, 2021
@jplatte
Copy link

jplatte commented Aug 6, 2021

#230 was merged, this can be closed.

@jyn514 jyn514 closed this as completed Aug 6, 2021
sunshowers added a commit to sunshowers/tabular-rs that referenced this issue Jan 6, 2022
This is no longer recommended: see
rust-lang/api-guidelines#229.
sunshowers added a commit to sunshowers/tabular-rs that referenced this issue Jan 6, 2022
This is no longer recommended: see
rust-lang/api-guidelines#229.
sunshowers added a commit to tabular-rs/tabular-rs that referenced this issue Jan 6, 2022
This is no longer recommended: see
rust-lang/api-guidelines#229.
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this issue May 9, 2022
tarcieri added a commit to RustCrypto/elliptic-curves that referenced this issue May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
amendment Amendments to existing guidelines 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. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. T-libs Relevant to the libraries subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants