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

ci: try running coverage for all OSes #1975

Merged
merged 1 commit into from
Nov 13, 2021

Conversation

davidhewitt
Copy link
Member

According to https://docs.codecov.com/docs/merging-reports if we upload multiple coverage reports they all get automatically merged. So I'm opening this PR to see what happens if I try that!

@davidhewitt davidhewitt force-pushed the multiple-os-coverage branch 6 times, most recently from 1335fb2 to f0d615c Compare November 13, 2021 07:41
@davidhewitt
Copy link
Member Author

davidhewitt commented Nov 13, 2021

Hmmm @taiki-e have you seen errors like these from running cargo-llvm-cov on windows before?

libpyo3-8bd03e2eb0b9a94d.rlib(pyo3-8bd03e2eb0b9a94d.pyo3.e32d66c5-cgu.2.rcgu.o) : fatal error LNK1227: conflicting weak extern definition for '_RNvNtCs9ZwDYBEfEGf_4pyo311type_object12get_tp_allocB3_'.  New default '.weak._RNvNtCs9ZwDYBEfEGf_4pyo311type_object12get_tp_allocB3_.default._RINvMNtCsk1Jcy3OMEV4_5alloc5sliceSh9to_vec_inNtNtB5_5alloc6GlobalECs9ZwDYBEfEGf_4pyo3' conflicts with old default '.weak._RNvNtCs9ZwDYBEfEGf_4pyo311type_object12get_tp_allocB3_.default._RINvMNtCsjJUIXhzXbX_4core3stre13get_uncheckedINtNtNtB5_3ops5range5RangejEECs9ZwDYBEfEGf_4pyo3' in libpyo3-8bd03e2eb0b9a94d.rlib(pyo3-8bd03e2eb0b9a94d.pyo3.e32d66c5-cgu.9.rcgu.o)

I can reproduce locally with latest nightly if I run cargo llvm-cov --package pyo3 pyo3-build-config pyo3-macros-backend pyo3-macros --no-report on my Windows machine.

@taiki-e
Copy link
Contributor

taiki-e commented Nov 13, 2021

@davidhewitt That is a rustc bug: rust-lang/rust#85461

I'll implement a workaround on the cargo-llvm-cov side.

@davidhewitt
Copy link
Member Author

Aha brilliant, thank you!

@taiki-e
Copy link
Contributor

taiki-e commented Nov 13, 2021

@davidhewitt I've released cargo-llvm-cov 0.1.11, which implements a workaround for that issue. Could you try that version?

Comment on lines 300 to 317
- if: matrix.os.name == 'windows'
name: install cargo-llvm-cov (windows)
shell: powershell
run: |
wget https://github.com/taiki-e/cargo-llvm-cov/releases/download/v${CARGO_LLVM_COV_VERSION}/cargo-llvm-cov-x86_64-unknown-linux-gnu.tar.gz -qO- | tar -xzvf -
Invoke-WebRequest https://github.com/taiki-e/cargo-llvm-cov/releases/download/v${Env:CARGO_LLVM_COV_VERSION}/cargo-llvm-cov-${Env:CARGO_LLVM_COV_ARCH}.zip -OutFile cargo-llvm-cov.zip
Expand-Archive cargo-llvm-cov.zip
mv ./cargo-llvm-cov/cargo-llvm-cov.exe ~/.cargo/bin
- uses: actions-rs/toolchain@v1
with:
toolchain: nightly
override: true
profile: minimal
components: llvm-tools-preview
- if: matrix.os.name != 'windows'
name: install cargo-llvm-cov (unix)
run: |
wget https://github.com/taiki-e/cargo-llvm-cov/releases/download/v${CARGO_LLVM_COV_VERSION}/cargo-llvm-cov-${CARGO_LLVM_COV_ARCH}.tar.gz -qO- | tar -xzvf -
mv cargo-llvm-cov ~/.cargo/bin
Copy link
Contributor

@taiki-e taiki-e Nov 13, 2021

Choose a reason for hiding this comment

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

Btw, cargo-llvm-cov has both .tar.gz and .zip distribution on windows, so you can unify the installation script on both windows and unix (with shell: bash). For example:

host=$(rustc -Vv | grep host | sed 's/host: //')
curl -fsSL https://github.com/taiki-e/cargo-llvm-cov/releases/download/v${CARGO_LLVM_COV_VERSION}/cargo-llvm-cov-"$host".tar.gz | tar xzf - -C ~/.cargo/bin

@davidhewitt
Copy link
Member Author

Thanks @taiki-e, the windows job working great now!

I think this looks like codecov is merging reports? Not super clear, I'm going to go ahead and merge this, might play around more another day.

@davidhewitt davidhewitt merged commit 3044bd9 into PyO3:main Nov 13, 2021
@davidhewitt davidhewitt deleted the multiple-os-coverage branch November 13, 2021 11:04
@taiki-e
Copy link
Contributor

taiki-e commented Feb 12, 2022

I think this looks like codecov is merging reports? Not super clear, I'm going to go ahead and merge this, might play around more another day.

This may be due to the different path between windows and unix. (I feel it's a bit odd that codecov can't handle it, but we may need to do what cargo-llvm-cov's test is doing)

@davidhewitt
Copy link
Member Author

Hmm interesting. We're not generating JSON coverage here though; we're using --lcov. I don't know much about lcov format and how easy it would be to do that replace.

I have since noticed that codecov does update the coverage number reported in the PR as the separate jobs finish, so it's quite possible it is actually doing the right thing already.

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