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

Cannot set cargoExtraArgs in craneLib.cargoNextest (#675) #678

Merged
merged 8 commits into from
Aug 6, 2024

Conversation

skarmux
Copy link
Contributor

@skarmux skarmux commented Aug 5, 2024

Motivation

To resolve issue (#675) with the cargoExtraArgs attribute in craneLib.cargoNextest being placed in between cargo <HERE> nextest or cargo <HERE> llvm-cov nextest where there should be no options/flags.

Cargo flags cannot be used in this context. Only cargo nextest run/archive flags, which contain all the flags from cargo test (at least) are possible.

Sources I have checked:

Checklist

  • added tests to verify new behavior
  • added an example template or updated an existing one
  • updated docs/API.md (or general documentation) with changes
  • updated CHANGELOG.md

Both `cargo nextest run` and `cargo llvm-cov` do not place any cargo
options/flags right behind `cargo <HERE> ...`. All cargo related flags
can be added to the `cargoNextestExtraArgs` attribute. [Issue ipetkov#675]
After altering `checkPhaseCargoCommand` to use one of two explicit
command strings inside if/else blocks, it is no longer necessary to
default `cmd` to an empty value when `withLlvmCov` is set to `true`.
@skarmux
Copy link
Contributor Author

skarmux commented Aug 5, 2024

I have made the decision to alter the creation of checkPhaseCargoCommand a little further and use if/else blocks to make it more obvious that cargo nextest run and cargo llvm-cov nextest are two distinct programs, although they share some wording and one is basically a wrapper for the other.

Since I'm still a noob with Nix, there might be benefits of using optional strings over if/else that I don't know of. Up for discussion, I guess. 😄

@skarmux skarmux marked this pull request as ready for review August 5, 2024 14:22
lib/cargoNextest.nix Outdated Show resolved Hide resolved
@ipetkov
Copy link
Owner

ipetkov commented Aug 6, 2024

You can ignore the build-std failure, that was fixed in d0c8f4e and should go away with a merge/rebase with master

Copy link
Owner

@ipetkov ipetkov left a comment

Choose a reason for hiding this comment

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

Thanks again for reporting and fixing this!

@ipetkov ipetkov merged commit 4c6c779 into ipetkov:master Aug 6, 2024
19 checks passed
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.

Cannot set cargoExtraArgs in craneLib.cargoNextest
2 participants