-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Pass --cfg doc to check builds of dependencies as part of cargo doc/rustdoc #8811
Comments
Also, I've marked this a bug, because of the unexpected behavior it produces, but I could see the argument to call it a feature request. |
See also rust-lang/rust#76252. |
Note that crates have a tendency to do very cursed things when cfg(doc) is passed (https://doc.rust-lang.org/nightly/rustdoc/advanced-features.html#interactions-between-platform-specific-docs, rust-lang/rust#75100), so this will break crates in practice. I think the tradeoffs are worth it (since some crates will be broken either way), but any changes here should at least get a crater run. See also the discussion in rust-lang/rust#77276 (comment). |
it does not work to have these feature guards currently until rustc issue #8811 ([1]) is fixed since we just get errors running `cargo test` or `cargo doc` otherwise. the problem stems from the fact that `--cfg doc` will be used for compiling the main crate being processed but not its dependencies, thus here the re-exports are enabled in the binding crate, but not the actual items in the sys crate, and thus 'unresolved import' errors occur, complaining about not being able to find the items in the sys crate. note, it obviously would not hurt to just have these string constants unguarded, but i felt that it would be perhaps nicer to use the guards and associated feature documentation to highlight the version requirement for the use of these two significant properties. [1]: rust-lang/cargo#8811
My team also suffers from similar problem. We are using different rustc version and nightly releases for generating documentation, mainly because they are more feature-rich at the moment. However, that makes some static assertions false, which we would rather avoid making in the first place when generating docs. @ehuss Would the Cargo team be open to accepting contributions on this topic? Or do you feel like there is more discussion needed? |
The team discussed this a bit, and I think there is still quite a bit of open design exploration needed. Some thoughts and questions:
|
These are some fantastic thoughts. Thanks for taking the time to discuss. I hadn't thought of the caching problem and it is indeed super important to keep these caches working. It gets me thinking that maybe this problem is better solved in the compiler. The #[cfg(all(not(doc),LLVM_SYS_NOT_FOUND))]
std::compile_error!(/*...*/) This approach runs into problems because there's no cfg flag for |
`#[cfg(doc)]` could change the interface and thus fail the build. Refs: * rust-lang/cargo#8811 * rust-lang/cargo#2025 changelog: changed
# Objective - Fixes #14697 ## Solution This PR modifies the existing `all_tuples!` macro to optionally accept a `#[doc(fake_variadic)]` attribute in its input. If the attribute is present, each invocation of the impl macro gets the correct attributes (i.e. the first impl receives `#[doc(fake_variadic)]` while the other impls are hidden using `#[doc(hidden)]`. Impls for the empty tuple (unit type) are left untouched (that's what the [standard library](https://doc.rust-lang.org/std/cmp/trait.PartialEq.html#impl-PartialEq-for-()) and [serde](https://docs.rs/serde/latest/serde/trait.Serialize.html#impl-Serialize-for-()) do). To work around rust-lang/cargo#8811 and to get impls on re-exports to correctly show up as variadic, `--cfg docsrs_dep` is passed when building the docs for the toplevel `bevy` crate. `#[doc(fake_variadic)]` only works on tuples and fn pointers, so impls for structs like `AnyOf<(T1, T2, ..., Tn)>` are unchanged. ## Testing I built the docs locally using `RUSTDOCFLAGS='--cfg docsrs' RUSTFLAGS='--cfg docsrs_dep' cargo +nightly doc --no-deps --workspace` and checked the documentation page of a trait both in its original crate and the re-exported version in `bevy`. The description should correctly mention for how many tuple items the trait is implemented. I added `rustc-args` for docs.rs to the `bevy` crate, I hope there aren't any other notable crates that re-export `#[doc(fake_variadic)]` traits. --- ## Showcase `bevy_ecs::query::QueryData`: <img width="1015" alt="Screenshot 2024-08-12 at 16 41 28" src="https://github.com/user-attachments/assets/d40136ed-6731-475f-91a0-9df255cd24e3"> `bevy::ecs::query::QueryData` (re-export): <img width="1005" alt="Screenshot 2024-08-12 at 16 42 57" src="https://github.com/user-attachments/assets/71d44cf0-0ab0-48b0-9a51-5ce332594e12"> ## Original Description <details> Resolves #14697 Submitting as a draft for now, very WIP. Unfortunately, the docs don't show the variadics nicely when looking at reexported items. For example: `bevy_ecs::bundle::Bundle` correctly shows the variadic impl: ![image](https://github.com/user-attachments/assets/90bf8af1-1d1f-4714-9143-cdd3d0199998) while `bevy::ecs::bundle::Bundle` (the reexport) shows all the impls (not good): ![image](https://github.com/user-attachments/assets/439c428e-f712-465b-bec2-481f7bf5870b) Built using `RUSTDOCFLAGS='--cfg docsrs' cargo +nightly doc --workspace --no-deps` (`--no-deps` because of wgpu-core). Maybe I missed something or this is a limitation in the *totally not private* `#[doc(fake_variadic)]` thingy. In any case I desperately need some sleep now :)) </details>
Problem
Some packages such as
llvm-sys
have workarounds that allow their docs to build on docs.rs despite the fact that no usable binary can be generated (in this case because it relies on a platform binary that is not present on docs.rs). Essentially it uses detection of#[cfg(doc)]
to allow builds to succeed when they would otherwise fail. This works well today.However, when this project is a dependency in another project,
cargo doc
will not just runrustdoc
, it will also invokerustc
to perform a check build. This bypasses any#[cfg(doc)]
protections because it runs without--cfg doc
. It is currently not possible to detect that your crate is being compiled incheck
mode as part of a largerdoc
run. The desired build characteristics, however, are the same. We don't need a working binary, and we're invoking the compiler for the purpose of docs.It's also very strange to have a case where
cargo doc
works on a project but not when that project is a dependency.I've produced a minimal example of this that demonstrates the issue without all the cruft of thinking about LLVM and system dependencies.
Steps
djrenren/cargo-doc-failure
cd app
cargo rustdoc
Notice this fails while
cargo rustdoc
in the dependency succeeds.Current Workarounds
Local workaround:
$ RUSTFLAGS="--cfg doc" cargo doc
Docs.rs workaround:
Cargo.toml:
Suggested Solution(s)
As suggested in the title of the issue, pass the doc config flag in while doing check builds as part of
cargo rustdoc
Provide a standard way to add
cfg
flags to libraries when built as part of rustdoc. That is, standardize what docs.rs already does so that it works locally as well.Notes
This occurs on all cargo versions I've been able to find so the version is largely irrelevant, but let's say latest stable 1.47.0
Passing a doc flag by default to check builds could produce strange results if downstream crates use doc cfg's to change the interface of their crate, which may be a point in favor of solution 2.
The text was updated successfully, but these errors were encountered: