fix cargo hack job for bin target

[Tudyx]

cargo hack --feature-powerset check --lib --tests failed for workspace that contains a package with only bin target(s). --all-targets flag resolves this

Example of error:

error: no library targets found in package `foo`

[jonhoo]

The cargo hack check makes less sense for binary-only packages since in general for binary packages the feature-unionification that Cargo does is less relevant. Instead what I think I'd want to do is to only run this step if there are lib crates present. We may have to do a bit of querying using cargo metadata here to get it to do the right thing automatically. Alternatively, just remove that step from your CI if your crate only has binaries :p

I think a comment would at least be useful for this step explaining that it should be removed if you don't have library crates.

Where it gets weird is if you have a workspace with two crates, only one of which has a lib. In that case, I honestly don't know what the right way to achieve this is beyond going and calling this command once in every sub-crate with automation to decide which ones to skip. In a sense, it feels wrong for cargo to fail here in the first place. Curious to hear your take on this @taiki-e?

Also, from memory, I used to have this as --all-targets, but changed it because it made things awkward for things like nightly-only benchmarks and binaries with required-features. And since the library is the main thing you want to check feature combinations of anyway, --lib felt more "right".

[taiki-e]

There seem to be several different issues/questions here.

Should cargo-hack (or cargo?) prevent the --lib flag from passing to a crate without the library part?

Where it gets weird is if you have a workspace with two crates, only one of which has a lib. In that case, I honestly don't know what the right way to achieve this is beyond going and calling this command once in every sub-crate with automation to decide which ones to skip. In a sense, it feels wrong for cargo to fail here in the first place.

The current behavior is not to allow it anyway (error is emitted by cargo).

Perhaps the preferable behavior would be to:

• If there are no crates with the library part in the crates to be built: emit an error or a warning
  • I have no strong opinion on: warning vs error
• If there are one or more crates with the library part in the crates to be built:
  • If the build target flag is --lib only: skip the crates without the library part and emit a message indicating it.
  • If other build target flags (e.g., --tests) are also passed: ignore the --lib flag for crates without the library part and emit a message indicating it.
    • But reading Jon's comment, you seem to suggest ignoring crates without the library part in this case as well?

      I think I'd want to do is to only run this step if there are lib crates present

Should we do a feature check even on binary crate?

The cargo hack check makes less sense for binary-only packages since in general for binary packages the feature-unionification that Cargo does is less relevant. Instead what I think I'd want to do is to only run this step if there are lib crates present. We may have to do a bit of querying using cargo metadata here to get it to do the right thing automatically. Alternatively, just remove that step from your CI if your crate only has binaries :p

I think it may be true that many binary crates do not have feature flags, but I don't know of any justification for not doing a feature check on a binary crate with feature flags.

That said, I noticed that I'm also using --lib for the crate with library and binary. (Although in that crate, the binary is only built when all features are enabled, so it is not a problem.)

Which build targets do we need to do a feature check on? (--lib/--bins vs --tests vs --all-targets, etc.)

IMO, only libraries and binaries (with feature flags) need (strict) feature check in CI. (In other words, only the parts published on crates.io and used by users in the usual way (as dependencies or as binaries installed by cargo install))

I have explained several times in the past why I do not do feature checks for tests: crossbeam-rs/crossbeam#840 (comment), rust-lang/futures-rs#2216 (TL;DR: I don't see any way to properly check it or a sufficient reason for doing it)

Also, at least for v1 resolver, the feature checks for build targets that enable dev-dependencies must be separate from the feature checks for library or binary. Otherwise (even with cargo-hack) feature checks for library or binary will not function properly due to feature unification.

---

EDIT: Therefore, I think a reasonable default is to not pass any build target flags.

[jonhoo]

Thank you for the detailed analysis!

I do think Cargo's behavior could be improved here. At least to me, this is similar to rust-lang/cargo#10508; if the user isn't specifying a particular name (e.g., --test foo or --bin bar), then Cargo should just warn if the specifier matches nothing, but not fail. Which I think also align with what you're proposing. In a workspace, the result would be that anything that does have a lib gets the lib checked, whereas anything that does not will warn but succeed.

As for feature checking binaries, I honestly can't remember what I ran into. I suspect it was because I was also passing --all-targets at the time, which probably broke because of benchmarks. I think your suggestion is a good one of just running plain old cargo check with no additional flags.

Your point about not checking tests is new to me, but I think I agree it makes sense.

---

Which is all to say, I agree. Let's update the invocation to just be

cargo hack --feature-powerset check

Not sure if --workspace should also be included?

And then if someone wants a different behavior for a particular package they can just override.

[ryoqun]

cargo hack --feature-powerset check --lib --tests failed for workspace that contains a package with only bin target(s). --all-targets flag resolves this

Example of error:

error: no library targets found in package `foo`

Fyi, i'm trying to demote this error into just a warn, which will solve the root problem altogether: rust-lang/cargo#14163 upstream bug: rust-lang/cargo#10958

that said, I'd say these philosophical discussions were interesting to me: #4 (comment)