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

xtask: add coverage command #2067

Merged
merged 3 commits into from
Dec 27, 2021
Merged

Conversation

davidhewitt
Copy link
Member

This is a bit of an experiment to add a cargo xtask coverage alias.

Ultimately this will re-implement the coverage recipe in ci. I also was playing around with adding coverage from out examples and python tests. It doesn't seem to be working quite yet; needs upstream support in cargo-llvm-cov which I'm still trying to figure out.

Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Thanks :)

I've played around a bit with this "xtask' approach on a project and I like it. I'd be happy to see this go forward. And to help implement it ❤️

@@ -119,7 +119,8 @@ harness = false
members = [
"pyo3-macros",
"pyo3-macros-backend",
"examples"
"examples",
"xtask"
Copy link
Member

Choose a reason for hiding this comment

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

We don't necessarily need to name this "xtask". I don't have a motivation for changing it. But we could.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I don't personally love the name xtask but I do like that it's a documented "approach" (https://github.com/matklad/cargo-xtask), so without good motivation I don't feel a need to change it.

For example I'd personally enjoy the alias being called cargo run-script. There's nothing stopping us adding a couple extra aliases that all do the same later, if we wanted ;)

xtask/Cargo.toml Outdated

[dependencies]
anyhow = "1.0.51"
clap = { version = "3.0.0-rc.7", features = ["derive"] }
Copy link
Member

Choose a reason for hiding this comment

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

Does this add clap to pyo3's dependency tree? If so can we avoid that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't believe it does, no. This is an excerpt of Cargo.lock from a test project built against this branch:

[[package]]
name = "pyo3"
version = "0.15.1"
dependencies = [
 "cfg-if",
 "indoc",
 "libc",
 "num-bigint",
 "parking_lot",
 "paste",
 "pyo3-build-config",
 "pyo3-macros",
 "unindent",
]

So clap hasn't come through as a dependency here.

@davidhewitt davidhewitt marked this pull request as ready for review December 26, 2021 22:40
@davidhewitt
Copy link
Member Author

Great! I've tidied this up a bit and added cargo xtask test-py, which now gets used in CI instead of make test_py.

If CI passes I'll merge this now; I'll follow up on the coverage command improvements later once the cargo-llvm-cov support is finished, and @mejrs please do feel welcome to add to and expand to the set of commands implemented here to replace the Makefile :)

@davidhewitt davidhewitt merged commit 19ecd17 into PyO3:main Dec 27, 2021
@davidhewitt davidhewitt deleted the xtask-coverage branch December 27, 2021 09:48
bors bot added a commit to taiki-e/cargo-llvm-cov that referenced this pull request Jan 1, 2022
115: cli: add show-env subcommand r=taiki-e a=davidhewitt

Implements the `show-env` suggestion from #93 (comment)

An example output:

```
$ cargo llvm-cov show-env
RUSTFLAGS=" -Z instrument-coverage --remap-path-prefix /home/david/dev/pyo3/= --cfg coverage --cfg trybuild_no_target"
LLVM_PROFILE_FILE="/home/david/dev/pyo3/target/llvm-cov-target/pyo3-%m.profraw"
CARGO_INCREMENTAL="0"
CARGO_TARGET_DIR="/home/david/dev/pyo3/target/llvm-cov-target"
```

After exporting these vars, I had a go at building pyo3's `cdylib` examples and then running them with `pytest`.

Pleased to see that I do get profile output in the expected location:

```
$ ls target/llvm-cov-target/
CACHEDIR.TAG                         pyo3-6622988374613674511_0.profraw
debug                                pyo3-8390305564713967804_0.profraw
pyo3-10144924340354712083_0.profraw  pyo3-8655209440434687052_0.profraw
pyo3-12448306348928428713_0.profraw  pyo3-9115749969700858718_0.profraw
pyo3-13212718739740305352_0.profraw  pyo3-9296542518232051450_0.profraw
pyo3-17208771140764127027_0.profraw  pyo3.profdata
pyo3-40338836728015828_0.profraw     release
pyo3-5761173678925874245_0.profraw   wheels
pyo3-6135639819960792837_0.profraw
```

There's still some pain points:
  1. I need to unset `CARGO_TARGET_DIR` before attempting to run `cargo llvm-cov` command, or I get crashes because it tries to append `llvm-cov-target` for a second time:
  
  ```
  $ cargo llvm-cov --no-run --summary-only
  error: no input files specified. See llvm-profdata merge -help
  error: failed to merge profile data: process didn't exit successfully: `/home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-profdata merge -sparse -o /home/david/dev/pyo3/target/llvm-cov-target/llvm-cov-target/pyo3.profdata` (exit status: 1)
  ```
  
  2. After unsetting `CARGO_TARGET_DIR`, I still get a failure to generate the report. I suspect this is because `cargo-llvm-cov` is not finding and passing the `cdylib` outputs to `llvm-cov report`?
  
  ```
  $ cargo llvm-cov --no-run --summary-only --verbose
     Running `/home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-profdata merge -sparse /home/david/dev/pyo3/target/llvm-cov-target/pyo3-10144924340354712083_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-12448306348928428713_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-13212718739740305352_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-17208771140764127027_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-40338836728015828_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-5761173678925874245_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-6135639819960792837_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-6622988374613674511_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-8390305564713967804_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-8655209440434687052_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-9115749969700858718_0.profraw /home/david/dev/pyo3/target/llvm-cov-target/pyo3-9296542518232051450_0.profraw -o /home/david/dev/pyo3/target/llvm-cov-target/pyo3.profdata`
     Running `/home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-cov report -instr-profile=/home/david/dev/pyo3/target/llvm-cov-target/pyo3.profdata -ignore-filename-regex '(^|/)(rustc/[0-9a-f]+|(\.)?cargo/(registry|git)|(\.)?rustup/toolchains|tests|examples|benches|target/llvm-cov-target)/|^/home/david/|^pyo3-macros/|^pyo3-macros-backend/|^pyo3-build-config/|^examples/|^xtask/'`
  No filenames specified!
  error: failed to generate report: process didn't exit successfully: `/home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-cov report -instr-profile=/home/david/dev/pyo3/target/llvm-cov-target/pyo3.profdata -ignore-filename-regex '(^|/)(rustc/[0-9a-f]+|(\.)?cargo/(registry|git)|(\.)?rustup/toolchains|tests|examples|benches|target/llvm-cov-target)/|^/home/david/|^pyo3-macros/|^pyo3-macros-backend/|^pyo3-build-config/|^examples/|^xtask/'` (exit status: 1)
   ```
   
If you're interested, you can see the companion PyO3 at PyO3/pyo3#2067



Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: Taiki Endo <[email protected]>
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.

2 participants