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

target.'cfg(debug_assertions)'.dependencies doesn't work as expected #7634

Closed
benediktwerner opened this issue Nov 27, 2019 · 6 comments · Fixed by #7660
Closed

target.'cfg(debug_assertions)'.dependencies doesn't work as expected #7634

benediktwerner opened this issue Nov 27, 2019 · 6 comments · Fixed by #7660
Labels
A-configuration Area: cargo config files and env vars A-diagnostics Area: Error and warning messages generated by Cargo itself. C-bug Category: bug

Comments

@benediktwerner
Copy link
Contributor

Problem
Specifying conditional dependencies using target.'cfg(debug_assertions)'.dependencies does not work as expected (i.e. only building the dependencies when debug assertions are enabled) and instead always includes the dependencies. The same applies for cfg(test) and cfg(proc_macro) although dev-dependencies can be used instead of cfg(test).

I guess it kind of makes sense that only cfg-attributes that have something to do with a target work in this context however it is still very confusing and unexpected. Also, the documentation explicitly mentions that cfg(feature = ...) doesn't work so I would expect everything else to work.

Steps
Using this Cargo.toml

[package]
name = "test"
version = "0.1.0"
authors = []
edition = "2018"

[target.'cfg(debug_assertions)'.dependencies]
color-backtrace = "0.3"

and compiling with cargo build --release still compiles color-backtrace.

Possible Solutions

  1. Clarify in the documentation that only cfg-attributes related to targets work in this context. In that case, it would be good to add another way to include dependencies only for debug or release builds e.g. using profile.dev.dependencies. (This might be a good idea in any case).
  2. Allow all cfg-attributes in the target context. The main problem why this currently doesn't work is that the cfg values are taken from rustc --print cfg which always includes debug_assertions. One option would be to call rustc using the correct attributes for optimization, crate type, etc. but this would require some changes as this is currently computed as part of TargetInfo which is independent of any flags related to profiles. Another option would be to fix the cfg list at the uses where profile flags would be available. Although this is kinda hacky it would be fairly easy to do as there are only 5 uses of the cfg list and they all have profile information available.

Personally I think solution 2 will always be a bit messy so it's probably best to add something like profile.dev.dependencies instead. I would be happy to implement this if it sounds like a good idea.

@benediktwerner benediktwerner added the C-bug Category: bug label Nov 27, 2019
@benediktwerner benediktwerner changed the title target.'cfg(debug_assertions)'.dependencies doesn't work as expected target.'cfg(debug_assertions)'.dependencies doesn't work as expected Nov 27, 2019
@ehuss
Copy link
Contributor

ehuss commented Nov 27, 2019

I think this is essentially a duplicate of #5777.

I'd be a little uneasy about trying to set debug-assertions in TargetInfo because there isn't one profile, but instead a profile per unit, based on various factors (cargo target, package, cli flags, etc.).

@benediktwerner
Copy link
Contributor Author

Yeah, you are right, it's basically the same issue. But that issue also doesn't have a solution so the question is still what the best solution for this would be.

As for setting debug_assertions in TargetInfo, that's exactly what I meant when I said it's independent of any flags related to profiles. It doesn't really make sense to include it there. I think the main question is if target.cfg(...).dependencies should even support all configuration values in the first place. Features already don't work after all. It looks like this wasn't really supposed to be a real cfg check but only a more advanced target selection with a pretty unfortunate syntax but now it's probably already too late to change that. However, in the end, this would of course also be more powerful, it just isn't really correct to put this under target.
But if we want to get proper cfg checking the best way would probably be to separate it out of TargetInfo. The thing is this also gets a bit nasty with stuff like cargo fetch which doesn't even care about profiles. It should probably ignore all attributes that aren't about targets.

@ehuss
Copy link
Contributor

ehuss commented Dec 1, 2019

I think it would be best for now if Cargo generated an error if it sees debug_assertions or feature=….

@ehuss ehuss added A-configuration Area: cargo config files and env vars A-diagnostics Area: Error and warning messages generated by Cargo itself. labels Dec 3, 2019
@benediktwerner
Copy link
Contributor Author

benediktwerner commented Dec 4, 2019

Ok, I guess that's the simplest solution for now. I'm going to have a shot at implementing that error and probably also create a PR to update the docs. What do you think about something like profile.dev.dependencies? Would it make sense to create a proposal/RFC or something for that?

@ehuss
Copy link
Contributor

ehuss commented Dec 4, 2019

I don't know how profile-specific dependencies would work. A single artifact can consist of multiple profiles, and a single invocation can do multiple things with different profiles. It seems like it would be difficult.

@benediktwerner
Copy link
Contributor Author

Interesting, I thought there was always only one profile active for a specific build/run. To me, it looks like the CompileOptions also only have a single profile. But whatever, I guess this isn't really that important.

@bors bors closed this as completed in 774d949 Dec 9, 2019
shssoichiro pushed a commit to shssoichiro/oxipng that referenced this issue Apr 24, 2023
* Make dependency on `image` optional

After PR #481 was merged, the
`image` dependency became unused when building with debug assertions
disabled, as it is only used to implement output sanity checks when such
assertions are enabled.

The `image` crate transitively pulls a significant amount of
dependencies, so it's useful for OxiPNG users to get rid of them when
not needed.

[Cargo does not allow specifying dependencies that are only pulled when
debug assertions are
enabled](rust-lang/cargo#7634), so the next
best way to give users some flexibility is to gate those debug
assertions behind a feature flag.

These changes add a `sanity-checks` feature flag that controls whether
the `image` crate and the related sanity checks are compiled in. This
feature is enabled by default to keep debug builds useful to catch
problems during development.

* Fix Clippy lints

* Run tests with new sanity-checks feature enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-diagnostics Area: Error and warning messages generated by Cargo itself. C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants