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

Only enable vergen on docsrs #533

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Only enable vergen on docsrs #533

merged 1 commit into from
Oct 9, 2023

Conversation

stefnotch
Copy link
Contributor

@stefnotch stefnotch commented Sep 26, 2023

Hopefully fixes #503

I personally would prefer if rustdoc fully supported including example code in the documentation, but this is better than nothing for now.

@@ -59,6 +59,9 @@ regex = ["dep:regex-automata"]
# Enable serde serialization support
serde = ["dep:serde"]

# Enable dependencies only needed for generation of documentation on docs.rs
docsrs = ["dep:vergen"]
Copy link
Owner

Choose a reason for hiding this comment

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

Have you checked that this actually gets enabled with generating docsrs docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't tested it with the live docsrs, since I'm not sure how one would go about doing so.

My understanding is that the docsrs feature gets enabled by

chumsky/Cargo.toml

Lines 67 to 69 in af4e2b1

[package.metadata.docs.rs]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this would enable such a feature. The cfg flag just allows you to do #[cfg(docsrs)] (which would probably be sufficient for your purposes, fwiw, instead of adding a cargo feature).

Copy link
Contributor Author

@stefnotch stefnotch Sep 27, 2023

Choose a reason for hiding this comment

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

Okay, I tried a few things in that direction, but haven't quite managed to get it to work as expected.

So I first tested the code with RUSTFLAGS="--cfg docsrs" cargo doc --open. (Because someone said that it's a workaround of some sort.) That only seems to pass the info to the Cargo.toml part. It doesn't pass the info to the lib.rs file, since #[cfg(docsrs)] in lib.rs had no effect.

Then I tested it with cargo rustdoc --open -- --cfg docsrs. This lets me do #[cfg(docsrs)] in lib.rs, but does not pass it to the build.rs script. And importantly, it doesn't seem to let me make vergen an optional feature.

I could probably find a variation where I get it to work, but it might almost be easier to make the docsrs stuff a feature. all-features = true should turn on the docsrs feature. Unless of course, you've got a good idea. I'm definitely willing to try out more stuff.

Copy link
Owner

Choose a reason for hiding this comment

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

Have you tried the command at the top of guide.rs?

RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --all-features

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed that command, thank you.

So I tested it just now:
Conditionally compiling vergen does not seem to be possible with

[target.'cfg(docsrs)'.build-dependencies]
vergen = { version = "=8.1.1", features = ["git", "gitoxide"] }

Conditionally compiling it with --all-features and then docsrs = ["dep:vergen"] does seem to work.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, that's unfortunate :(

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, not sure I understood this message. So this does work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this pull request should work.

@zesterer zesterer merged commit 7131ca4 into zesterer:main Oct 9, 2023
4 checks passed
@zesterer
Copy link
Owner

zesterer commented Oct 9, 2023

Thanks for the PR!

@stefnotch
Copy link
Contributor Author

I'm assuming that the new version on crates.io includes this PR?

The links seem to be broken now, they point at https://github.com/zesterer/chumsky/blob/VERGEN_IDEMPOTENT_OUTPUT/examples/brainfuck.rs, and I can't quite tell why.

https://docs.rs/chumsky/1.0.0-alpha.5/chumsky/index.html#example-brainfuck-parser

@zesterer
Copy link
Owner

zesterer commented Oct 9, 2023

It doesn't include this PR, the new version was released yesterday.

@stefnotch
Copy link
Contributor Author

@zesterer I see. So was the vergen approach always doomed to fail on docs.rs? Or does that call for some tricky investigation?

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.

Heavyweight vergen
2 participants