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

Add --cfg docsrs to all rustdoc invocations #2390

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Jan 23, 2024

Fixes #2389

@Urgau Urgau requested a review from a team as a code owner January 23, 2024 10:00
@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Jan 23, 2024
crates/metadata/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

Changes look good to me, would be good to hear if @syphar can think of any reason to not do this.

Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

I agree, from what I see this has become the de-facto standard, so setting it by default is a good idea.

You could see this as a breaking change, but only if

  • someone used the feature name docsrs for something else, or
  • the feature exists for future usage, but shouldn't be used yet.

Also having @epage on this topic makes me lean towards seeing these as edge-cases, and merging & deploying this.

@epage
Copy link

epage commented Jan 31, 2024

Good thinking through those cases and I agree that those are edge enough cases that for a service like docs.rs it seems fine for moving forward.

@syphar syphar merged commit 240d3e8 into rust-lang:master Feb 1, 2024
15 checks passed
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Feb 1, 2024
bors added a commit to rust-lang/cargo that referenced this pull request Feb 1, 2024
Add `docsrs` cfg as a well known `--check-cfg`

Now that rust-lang/docs.rs#2390 has been merged we can add the `docsrs` cfg in Cargo well known --check-cfg "list". The `docsrs` cfg used by at least [3k project on GitHub](https://github.com/search?q=lang%3Atoml+%2Frustdoc-args+%3D+%5C%5B%22--cfg%22%2C+%22docsrs%22%5C%5D%2F+NOT+is%3Afork&type=code&repo=&langOverride=&start_value=1) alone; including the cfg will help reduce the impact of enabling by default this feature.

> We include it here (in Cargo) instead of rustc, since there is a much closer relationship between Cargo and docs.rs than rustc and docs.rs. In particular, all users of docs.rs use Cargo, but not all users of rustc (like Rust-for-Linux) use docs.rs.

This is part of the last remaining bits of the `--check-cfg` feature.

r? `@epage`
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Feb 14, 2024
jplatte added a commit to ruma/ruma that referenced this pull request May 29, 2024
- Don't instruct docs.rs to set it, it does so by default now:
  rust-lang/docs.rs#2390
- Because of that, check-cfg also recognizes it and does not need
  extra configuration either.
jplatte added a commit to ruma/ruma that referenced this pull request May 30, 2024
- Don't instruct docs.rs to set it, it does so by default now:
  rust-lang/docs.rs#2390
- Because of that, check-cfg also recognizes it and does not need
  extra configuration either.
zydou pushed a commit to zydou/arti that referenced this pull request Aug 7, 2024
Don't need to tell docs.rs to enable `docsrs` cfg. It does it automatically as of rust-lang/docs.rs#2390 (comment)

While this change isn't in our MSRV yet, we were only using this when building for docs.rs, where we use the latest anyway.

See merge request tpo/core/arti!2308
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.

Native support for docs.rs conditional compilation
4 participants