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

Building fails with vendored dependencies #20

Closed
v-morlock opened this issue Sep 20, 2023 · 8 comments · Fixed by #21
Closed

Building fails with vendored dependencies #20

v-morlock opened this issue Sep 20, 2023 · 8 comments · Fixed by #21

Comments

@v-morlock
Copy link

Hi, when building a crate which depends on this crate to generate its docs, building fails when using "vendored" dependencies, as cargo will normalize the Cargo.toml and won't create Cargo.toml.orig. Is there anything i can do about that?

@ogoffart
Copy link
Member

What is the exact error?

Perhaps when no documentation is found, we could chose to either ignore it, or try to emit a warning. Or generate a text explaining the documentation were not found.

The mitigation is to make the crate optional as explained https://docs.rs/document-features/latest/document_features/#compatibility and to not enable this when building the docs as vendored

@oscartbeaumont
Copy link

Yeah, I think this is my fault for not correctly setting the dependency as optional in Specta as specified in the docs. Must have just missed it. 🤦

@ErichDonGubler
Copy link

ErichDonGubler commented Dec 21, 2023

The proposed mitigation to gate document-feature behind a feature works to resolve this issue, assuming that one has control over their dependencies or the cycles to fork a potentially deep dependency tree. Even with full control, however, this can still be a particularly time-consuming problem to solve, as the compilation failure can happen at a large distance between the crate depending on document-features and the workspace in which crates are being vendored in (see also RainbowCookie32/rusty-psn#172). We in Firefox ran into this issue while cargo vendor-ing wgpu:090f2f757c2d21a36c3bec1c0f43dc56aa9b9dd3, which expects to run on Rust 1.70 or higher. This assumption of minimum Rust version would seem to render the current Compatibility section's contents irrelevant for most readers. For convenience, the contents state (paraphrased):

The minimum Rust version required to use this crate is Rust 1.54 because of the feature to have macro in doc comments. You can make this crate optional and use #[cfg_attr()] statements to enable it only when building the documentation: You need to have two levels of cfg_attr because Rust < 1.54 doesn’t parse the attribute otherwise.

I think, for now, the best thing to do is to remove the error case entirely, and accept an empty set of features.

@ErichDonGubler
Copy link

One might reasonably suggest using a warning instead of an error in this case. Unfortunately, warning-level diagnostics in procedural macros are gated behind rust-lang/rust#54140 as Experimental portions of the proc_macro::Diagnostic API. For now, this is not an option.

@ErichDonGubler
Copy link

Just filed a Cargo issue to see if we might be able to resolve this upstream: rust-lang/cargo#13191

@ErichDonGubler
Copy link

Ed Page from the Cargo team recommends that fallback to Cargo.toml.orig be attempted when no features are found to be documented. This file is emitted when cargo package and cargo vendor "normalize" files and lose commentary in the way that motivates the OP issue: rust-lang/cargo#13191 (comment)

Ed also points to rust-lang/cargo#4956 as a place for interesting discussion, where there is a proposal to add TOML fields for feature descriptions.

@ogoffart
Copy link
Member

the crate already fallback to the .toml.orig file. In your case, that file probably does not exist.

I've made it an error because I thought someone using the crate without any feature would mean something is wrong and the programmer should be notified (eg, parse error on our misuse of the crate).
But in this case, I realize this is too strong. You're right that this should be a warning instead, or just generate some documentation text saying no features was found.

ErichDonGubler added a commit to ErichDonGubler/wgpu that referenced this issue Dec 22, 2023
Re-implements gfx-rs#4886 (CC @Wumpf)
without the `document-features` crate, which has issues integrating into
Firefox builds after being `cargo vendor`ed into its repository. This
issue is being tracked against
slint-ui/document-features#20. Once resolved,
I expect that we will want to revert this PR in its entirety, since
`document-features` is still a good addition to `wgpu`'s documentation
story.

Internally, consensus has already been achieved for this change.
Firefox's ability to build unfortunately take priority over this
particular convenience. Hopefully, we won't have to compromise shortly!

I tested this by ensuring that the HTML output of our existing
`document_features::document_features!(…)` usage was exactly the same.
There should be exactly zero regressions in the current state of
documentation for users. For maintainers, I have added a disclaimer that
one needs to keep changes in sync. with the relevant `Cargo.toml`
manifests.
ErichDonGubler added a commit to gfx-rs/wgpu that referenced this issue Dec 22, 2023
Re-implements #4886 (CC @Wumpf)
without the `document-features` crate, which has issues integrating into
Firefox builds after being `cargo vendor`ed into its repository. This
issue is being tracked against
slint-ui/document-features#20. Once resolved,
I expect that we will want to revert this PR in its entirety, since
`document-features` is still a good addition to `wgpu`'s documentation
story.

Internally, consensus has already been achieved for this change.
Firefox's ability to build unfortunately take priority over this
particular convenience. Hopefully, we won't have to compromise shortly!

I tested this by ensuring that the HTML output of our existing
`document_features::document_features!(…)` usage was exactly the same.
There should be exactly zero regressions in the current state of
documentation for users. For maintainers, I have added a disclaimer that
one needs to keep changes in sync. with the relevant `Cargo.toml`
manifests.
ErichDonGubler added a commit to ErichDonGubler/wgpu that referenced this issue Dec 22, 2023
Re-implements gfx-rs#4886 (CC @Wumpf)
without the `document-features` crate, which has issues integrating into
Firefox builds after being `cargo vendor`ed into its repository. This
issue is being tracked against
slint-ui/document-features#20. Once resolved,
I expect that we will want to revert this PR in its entirety, since
`document-features` is still a good addition to `wgpu`'s documentation
story.

Internally, consensus has already been achieved for this change.
Firefox's ability to build unfortunately take priority over this
particular convenience. Hopefully, we won't have to compromise shortly!

I tested this by ensuring that the HTML output of our existing
`document_features::document_features!(…)` usage was exactly the same.
There should be exactly zero regressions in the current state of
documentation for users. For maintainers, I have added a disclaimer that
one needs to keep changes in sync. with the relevant `Cargo.toml`
manifests.
ErichDonGubler added a commit to gfx-rs/wgpu that referenced this issue Dec 22, 2023
Re-implements #4886 (CC @Wumpf)
without the `document-features` crate, which has issues integrating into
Firefox builds after being `cargo vendor`ed into its repository. This
issue is being tracked against
slint-ui/document-features#20. Once resolved,
I expect that we will want to revert this PR in its entirety, since
`document-features` is still a good addition to `wgpu`'s documentation
story.

Internally, consensus has already been achieved for this change.
Firefox's ability to build unfortunately take priority over this
particular convenience. Hopefully, we won't have to compromise shortly!

I tested this by ensuring that the HTML output of our existing
`document_features::document_features!(…)` usage was exactly the same.
There should be exactly zero regressions in the current state of
documentation for users. For maintainers, I have added a disclaimer that
one needs to keep changes in sync. with the relevant `Cargo.toml`
manifests.
ogoffart added a commit that referenced this issue Dec 24, 2023
Because this could happen if the Cargo.toml is "normalized".
Instead, show a note in the documentation

Fixes #20
ogoffart added a commit that referenced this issue Dec 29, 2023
Because this could happen if the Cargo.toml is "normalized".
Instead, show a note in the documentation

Fixes #20
@ogoffart
Copy link
Member

Fixed in 0.2.8

ErichDonGubler added a commit to ErichDonGubler/wgpu that referenced this issue Feb 6, 2024
Revert "docs: inline `document-features` usage, remove dep."

This reverts commit 3d5bec6, with an
update to `document-features`, and preferring to keep new `feature`
content. To be clear, the only difference I have observed is the
addition of the `serde` feature.

In case it shortens anyone's search, the specific issue resolved is
[`slint-ui/document-features`#20](slint-ui/document-features#20).
ErichDonGubler added a commit to ErichDonGubler/wgpu that referenced this issue Feb 8, 2024
This reverts commit 3d5bec6, with an
update to `document-features`, and preferring to keep new `feature`
content. To be clear, the only difference I have observed is the
addition of the `serde` feature.

In case it shortens anyone's search, the specific issue resolved is
[`slint-ui/document-features`#20](slint-ui/document-features#20).
Wumpf pushed a commit to gfx-rs/wgpu that referenced this issue Feb 9, 2024
* docs: sync. `wgpu/Cargo.toml` feature comments with `lib.rs`

* Revert "docs: inline `document-features` usage, remove dep."

This reverts commit 3d5bec6, with an
update to `document-features`, and preferring to keep new `feature`
content. To be clear, the only difference I have observed is the
addition of the `serde` feature.

In case it shortens anyone's search, the specific issue resolved is
[`slint-ui/document-features`#20](slint-ui/document-features#20).
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 a pull request may close this issue.

4 participants