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

fix: add --coverage-target-only, to use rustflags only for target #153

Closed
wants to merge 1 commit into from

Conversation

haraldh
Copy link
Contributor

@haraldh haraldh commented Apr 25, 2022

Use CARGO_TARGET_{target}_RUSTFLAGS rather than RUSTFLAGS, if --target <TRIPLE> and --coverage-target-only is specified.

This is important, if the project uses multiple targets via the cargo bindeps feature, and not all targets can use instrument-coverage, e.g. a microkernel, or an embedded binary.

Signed-off-by: Harald Hoyer harald@profian.com

@taiki-e
Copy link
Owner

taiki-e commented Apr 25, 2022

Thanks for the PR. My concern is how this will interact with rustflags's priorities.

The cargo reference says:

There are three mutually exclusive sources of extra flags. They are checked in order, with the first one being used:

  1. RUSTFLAGS environment variable.
  2. All matching target..rustflags and target..rustflags config entries joined together.
  3. build.rustflags config value.

I would expect that if an unstable feature like bindeps is not enabled, simply removing the RUSTFLAGS environment variable would be enough (note: I have not tested), but I feel that approach would break when bindeps becomes stable.

I'm not sure how that would work if bindeps (or multideps in the future) is enabled.
cargo-llvm-cov just want to emulate the behavior of cargo, so we need to understand how cargo handles this when artifact-dependencies is enabled.

@haraldh
Copy link
Contributor Author

haraldh commented Apr 25, 2022

Do you want me to introduce a new option like --only-target?

@taiki-e
Copy link
Owner

taiki-e commented Apr 30, 2022

introduce a new option like --only-target

Introducing a new option seems to make sense to me.

Use `CARGO_TARGET_{target}_RUSTFLAGS` rather than `RUSTFLAGS`,
if `--target <TRIPLE>` and `--coverage-target-only` is specified.

This is important, if the project uses multiple targets via the cargo
`bindeps` feature, and not all targets can use `instrument-coverage`,
e.g. a microkernel, or an embedded binary.

Signed-off-by: Harald Hoyer <harald@profian.com>
@haraldh haraldh changed the title fix: use target rustflags fix: add --coverage-target-only, to use rustflags only for target May 19, 2022
@haraldh
Copy link
Contributor Author

haraldh commented May 19, 2022

introduce a new option like --only-target

Introducing a new option seems to make sense to me.

done

@haraldh
Copy link
Contributor Author

haraldh commented May 19, 2022

Moved to #167 ... sorry for the inconvenience, but I referenced the my branch name in other locations.

@haraldh haraldh closed this May 19, 2022
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.

None yet

2 participants