-
Notifications
You must be signed in to change notification settings - Fork 64
Allow building as dependency on docs.rs with no features enabled #199
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm reading this wrong, we already tried to enable the native-activity
backend by default for cfg(doc)
, i.e. a user runs cargo doc
in this repository.
Is that also not set on docs.rs for dependency crates?
@@ -1 +1,2 @@ | |||
target | |||
Cargo.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one, it looks like we deliberately didn't check one in but forgot to exclude it globally.
The other gitignore in the crate should be useless as these files are created at the workspace root, i.e. here. In light of that, can you prefix these with /
to ensure they only ignore this file in the root of the repository to match that?
// Avoid re-running build script if nothing changed. | ||
println!("cargo:rerun-if-changed=build.rs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "nothing" should be clarified to "avoid re-running the build script if anything but the build script changed".
At first I thought this was redundant, but supposedly the build script reruns on any file changes inside the package directory by default unless specified with rerun-if
- which won't be set without game-activity
. As such, changing the rest of the crate is not "nothing" but we still don't need to re-run the build script in that case.
// Whether this is used directly in or as a dependency on docs.rs. | ||
println!("cargo:rustc-check-cfg=cfg(used_on_docsrs)"); | ||
if std::env::var("DOCS_RS").is_ok() { | ||
println!("cargo:rustc-cfg=used_on_docsrs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe link the doc here that says that cfg(docsrs)
cannot be used because it's only set for the final/endpoint crate (i.e. winit
)?
#[cfg_attr(any(feature = "native-activity", doc), path = "native_activity/mod.rs")] | ||
#[cfg_attr(any(feature = "game-activity", doc), path = "game_activity/mod.rs")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little fuzzy on the old implementation here. Setting doc
would enable both path
attributes, which one takes precedence?
Additionally, this together with the redundant cfg
in the mod.rs
should have enabled things to work on docs.rs without features, which should at least set cfg(doc)
too?
In
winit
, we have to artificially enable an activity to get the docs to build on Android, which is kinda annoying:https://github.com/rust-windowing/winit/blob/969237f422e3d1d18821f4ee99e6be7bb6acd913/Cargo.toml#L125-L126
Instead, this PR proposes to handle that logic internally in
android-activity
, by relying on the fact that docs.rs sets an environment variable (see their docs).