Skip to content

Add cargo rustdoc for passing arbitrary flags to rustdoc#1977

Closed
Manishearth wants to merge 6 commits into
rust-lang:masterfrom
Manishearth:rustdoc_args
Closed

Add cargo rustdoc for passing arbitrary flags to rustdoc#1977
Manishearth wants to merge 6 commits into
rust-lang:masterfrom
Manishearth:rustdoc_args

Conversation

@Manishearth

Copy link
Copy Markdown
Member

Quite useful for doing cargo rustdoc -- --no-defaults to get a doc package with private items.

This is similar to cargo rustc, except that the flags passed to cargo rustdoc are applied to both the main package and dependencies.

A friend of mine was looking for this feature, couldn't find a way to do it without directly calling rustdoc. This would probably be useful for using the full configurability of rustdoc in cargo-applications.

r? @alexcrichton

@rust-highfive

Copy link
Copy Markdown

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@steveklabnik

Copy link
Copy Markdown
Contributor

conceputally, 👍 from me.

Comment thread src/bin/rustdoc.rs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this may want to customize this text instead of just copy/pasting from cargo help doc, for example this should probably start out by saying that it is intended to configure the invocation of rustdoc on one crate and not mention much about dependencies up here at the top.

@alexcrichton

Copy link
Copy Markdown
Member

cc @brson, @wycats, a new Cargo subcommand, but it fits well within the pattern of cargo rustc so I'm fine landing without an RFC.

@brson

brson commented Sep 11, 2015

Copy link
Copy Markdown
Contributor

sgtm

@Manishearth

Copy link
Copy Markdown
Member Author

So, I actually want the subcommand to pass the rustdoc flags to all dependencies. Rustdoc output would be weird if only one crate was compiled with a set of flags.

For cargo rustc it makes sense to limit it to the final crate, since it's mostly for tweaking linker args and whatnot. But for cargo rustdoc we want to finally get a uniform chunk of documentation, which means the flags should be passed down.

I thought this PR did do that, turns out it doesn't, I'll see if I can make it work so that the args are passed to upstream deps too.

(This is why I didn't add a check for multiple targets -- this should behave exactly like cargo doc, just that every rustdoc invocation gets the extra flags)

@alexcrichton

Copy link
Copy Markdown
Member

I would personally not want to land this with subtly different behavior than cargo rustc when both commands seem quite similar. Even with rustdoc I can imagine there are use cases of only passing flags to the top-level crate rather than all dependencies, although I'd certainly believe that it's more rare.

Configuration of rustdoc for all crates in the dependency graph is certainly desired, but I think that wants a more principled solution than just an extra command which passes all the flags everywhere. E.g. this is a recipe for all projects having their documentation building step be something like cargo rustdoc -- <ten obscure flags>, where instead the options could be stored in the manifest (for example) and the idiom would still be just cargo doc.

@Manishearth

Copy link
Copy Markdown
Member Author

Alright, fixed so that it mirrorrs rustc

E.g. this is a recipe for all projects having their documentation building step be something like cargo rustdoc -- , where instead the options could be stored in the manifest (for example) and the idiom would still be just cargo doc.

So there are multiple use cases here. One is for the default build, and one is for specialized builds. For example, cargo build might be the default, but I may want to occasionally be able to run cargo rustc -- -C codegen_units=10 or something during development. Or cargo rustc -- -Z extra-plugins=plugin -L /path/to/plugin -D foo. These aren't part of the build step, these are tweaks I'm running during development. Editing a file seems ... unwieldy, since these aren't part of the regular build.

I'd prefer if we had both a cargo-config based solution, and a terminal-based solution. The simplest one seems to be allowing cargo rustc and cargo rustdoc to have a --flag-all flag, which passes the provided flag to all deps (instead of the top one), and overrides any rustc-flags/rustdoc-flags in the cargo config.

@alexcrichton

Copy link
Copy Markdown
Member

Yes I agree that passing custom flags shouldn't always involve editing files, but I was mostly just saying that this should follow cargo rustc, discussion for this kind of feature probably shouldn't happen on this PR and it would likely require an RFC.

Comment thread src/bin/bench.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cargo tends to favor Vec::new() over vec![] currently

@Manishearth

Copy link
Copy Markdown
Member Author

(addressed)

Comment thread src/bin/rustdoc.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/compiler/rustdoc/

@bors

bors commented Sep 22, 2015

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #1991) made this pull request unmergeable. Please resolve the merge conflicts.

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.

7 participants