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

Enable feature(doc_auto_cfg) #1395

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Enable feature(doc_auto_cfg) #1395

merged 1 commit into from
Aug 8, 2023

Conversation

ctz
Copy link
Member

@ctz ctz commented Aug 8, 2023

This removes duplicated manual feature gates for documentation and leaves it to cargo doc to derive the same information from the actual feature gates.

I didn't find any gaps in the auto-generated features and what we had before, but now things like rustls::cipher_suite::TLS_ECDHE_* are correctly marked tls12-only.

fixes #1393

This removes duplicated manual feature gates for documentation
and leaves it to `cargo doc` to derive the same information from
the actual feature gates.

I didn't find any gaps in the auto-generated features and what we had
before, but now things like `rustls::cipher_suite::TLS_ECDHE_*`
are correctly marked tls12-only.
@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #1395 (3c9eea0) into main (f5837b0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1395   +/-   ##
=======================================
  Coverage   96.32%   96.32%           
=======================================
  Files          64       64           
  Lines       14734    14734           
=======================================
  Hits        14193    14193           
  Misses        541      541           
Files Changed Coverage Δ
rustls/src/client/client_conn.rs 88.32% <ø> (ø)
rustls/src/conn.rs 92.65% <ø> (ø)
rustls/src/server/server_conn.rs 87.41% <ø> (ø)
rustls/src/suites.rs 97.79% <ø> (ø)
rustls/src/verify.rs 63.63% <ø> (ø)
rustls/src/webpki/verify.rs 98.69% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ctz
Copy link
Member Author

ctz commented Aug 8, 2023

(i think we should do this in the webpki crate too)

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

This isn't from your diff, but I noticed while validating the change locally that we have a rustdoc warning:

warning: unresolved link to `ConfigBuilder::with_custom_certificate_verifier`
  --> rustls/src/builder.rs:71:10
   |
71 | ///  - [`ConfigBuilder::with_custom_certificate_verifier`] - requires dangerous_configuration feature flag
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the struct `ConfigBuilder` has no field or associated item named `with_custom_certificate_verifier`
   |
   = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default

Would you be open to fixing that in this branch? (also might be worth a CI tweak to fail closed on warnings.)

@ctz
Copy link
Member Author

ctz commented Aug 8, 2023

Would you be open to fixing that in this branch? (also might be worth a CI tweak to fail closed on warnings.)

Ah, so we do have -Dwarnings (deny warnings) on the cargo doc job, but also that uses --all-features (which makes ConfigBuilder::with_custom_certificate_verifier appear, and so the warning disppears).

The interesting question is whether there's a notation in cargo doc that means "link to this if it's compiled in"?

@cpu
Copy link
Member

cpu commented Aug 8, 2023

Ah, so we do have -Dwarnings (deny warnings) on the cargo doc job, but also that uses --all-features

Oooh, that makes sense.

@ctz
Copy link
Member Author

ctz commented Aug 8, 2023

Have raised rust-lang/rust#114626 to discuss that

@ctz ctz added this pull request to the merge queue Aug 8, 2023
Merged via the queue into main with commit 9bdb243 Aug 8, 2023
39 checks passed
@ctz ctz deleted the jbp-try-doc-auto-cfg branch August 8, 2023 14:37
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.

See if feature(doc_auto_cfg) works to avoid repeating feature gates
3 participants