Stop adding the --document-private-items flag if it's already being set #8422
Conversation
It is an error to define the flag twice, so if the unit is already setting that rustdoc flag, we shouldn't set it again, even if asked to do so.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Thanks for the PR! Do we have a precedent though of checking for these sorts of flags in |
|
I don't know if there are any precedents or policies, and I don't really have strong feelings about this fix, I was just trying to do something productive with 30 minutes of time waiting on a call and this bug seemed straightforward. :) I guess I would ask what the purpose of such a policy would be? I guess I could see the argument that we shouldn't be manipulating user-provided flags, because that would undermine the purpose of allowing them to be provided. But inspecting them? In this case, the user has specified, "I want this flag." And then Cargo, trying to be helpful, says, "I will add this flag, because I know that you probably want it!" And it was right, I did want it, but instead of being helpful, it breaks everything. Not only that, it's not obvious (as a user) why it adds it by default for some things and not others. So then to fix it, as a user (because presumably I added it to fix a case where it wasn't being specified by default and I wanted it) I have to either dig up the Cargo source code, or arbitrarily add it to specific targets until I get lucky or figure out that it's only the binary targets I shouldn't put it on. It seems like any policy that would make a user jump through those kind of hoops to prevent the tool from over-helping them is at least a little bit user-hostile for no discernible benefit, in my opinion. That said, on the other hand, you could probably make the case that this is actually a bug in |
|
AFAIK Cargo doesn't really do much introspection of |
|
I went ahead and created a PR for 'fixing' this at the |
|
Closing due to inactivity, and this seems to be blocked on the decision rust-lang/rust#73936. If it becomes clearer what happens with rustdoc, we can revisit this, but I think we will still probably prefer to avoid having Cargo inspecting rustc and rustdoc flags. |
|
Here's another case where this is frustrating: when there are both library and binary targets within the same package. Then I guess cargo doesn't currently have a way to pass flags to only certain crates within a package? |
Document rustfmt on nightly-rustc - Refactor the doc step for Rustdoc into a macro - Call the macro for both rustdoc and rustfmt - Add a `recursion_limit` macro to avoid overflow errors This does not currently pass --document-private-items for rustfmt due to rust-lang/cargo#8422 (comment). r? `@Mark-Simulacrum` cc `@calebcartwright`
…n514 Rustdoc: Change all 'optflag' arguments to 'optflagmulti' Because specifying these flags multiple times will never be discernibly different in functionality from specifying them a single time, there is no reason to fail and report an error to the user. This might be a slightly controversial change. it's tough to say, but it's hard to imagine a case where somebody was depending on this behavior, and doing this seem actively better for the user. This originally came up in discussion of a fix for [Cargo rust-lang#8373](rust-lang/cargo#8373), in [Cargo PR rust-lang#8422](rust-lang/cargo#8422). The issue is that Cargo will automatically add things like `--document-private-items` to binaries, because it's the only thing that makes sense there. Then some poor user comes along and adds `--document-private-items` to their `rustdoc` flags for the project and suddenly they're getting errors for specifying a flag twice and need to track down which targets to actually add it to without getting duplicates for reasons they won't understand without deep understanding of Cargo behavior. We're apparently hesitant to inspect `rustdoc` flags provided by the user directly in Cargo, because they're supposed to be opaque, so looking to see if it's already provided before adding it is evidently a non-starter. In trying to resolve that, one suggestion I came up with was to just change `rustdoc` to support passing the flag multiple times, because the user's intent should be clear and it's not *really* an error, so maybe this is a case of 'be permissive in what you accept'. This PR is an attempt to do that in a straightforward manner for purposes of discussion.
…n514 Rustdoc: Change all 'optflag' arguments to 'optflagmulti' Because specifying these flags multiple times will never be discernibly different in functionality from specifying them a single time, there is no reason to fail and report an error to the user. This might be a slightly controversial change. it's tough to say, but it's hard to imagine a case where somebody was depending on this behavior, and doing this seem actively better for the user. This originally came up in discussion of a fix for [Cargo rust-lang#8373](rust-lang/cargo#8373), in [Cargo PR rust-lang#8422](rust-lang/cargo#8422). The issue is that Cargo will automatically add things like `--document-private-items` to binaries, because it's the only thing that makes sense there. Then some poor user comes along and adds `--document-private-items` to their `rustdoc` flags for the project and suddenly they're getting errors for specifying a flag twice and need to track down which targets to actually add it to without getting duplicates for reasons they won't understand without deep understanding of Cargo behavior. We're apparently hesitant to inspect `rustdoc` flags provided by the user directly in Cargo, because they're supposed to be opaque, so looking to see if it's already provided before adding it is evidently a non-starter. In trying to resolve that, one suggestion I came up with was to just change `rustdoc` to support passing the flag multiple times, because the user's intent should be clear and it's not *really* an error, so maybe this is a case of 'be permissive in what you accept'. This PR is an attempt to do that in a straightforward manner for purposes of discussion.
It is an error to define the flag twice, so if the unit is already setting that rustdoc flag, we shouldn't set it again, even if asked to do so.
Fixes #8373