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

Add cargo-tarpaulin support #42

Closed
wants to merge 1 commit into from
Closed

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Jan 5, 2023

Given that actions-rs/tarpaulin is currently broken, it'd be nice for folks to have a migration path.

This will conflict with #41 -- I wasn't sure if you preferred I do this on top of that branch, or what, but this is cleaner in case you have feedback/changes you'd like me to make to that one. I'll rebase after that merges, or you can push the fix to my branch if I'm not around (I don't care much).

Note: cargo-tarpaulin only provides binaries for x86_64-unknown-linux-gnu, and the tarballs have fairly idiosyncratic names. It's plausible that will have to change if cargo-tarpaulin ever provides other binaries, but at the moment it does not, so who knows.

@thomcc thomcc force-pushed the cargo-tarpaulin branch 2 times, most recently from d0cfdd4 to a14d5af Compare January 5, 2023 09:49
@NobodyXu
Copy link
Collaborator

NobodyXu commented Jan 5, 2023

Note: cargo-tarpaulin only provides binaries for x86_64-unknown-linux-gnu, and the tarballs have fairly idiosyncratic names. It's plausible that will have to change if cargo-tarpaulin ever provides other binaries, but at the moment it does not, so who knows.

P.S. cargo-quickinstall provides builds for x86_64-pc-windows-msvc and x86_64-apple-darwin.

@thomcc
Copy link
Contributor Author

thomcc commented Jan 5, 2023

I removed it from the container CI. It fails on most of them due to

I could also duplicate the debian:11-slim job if you'd like.

@thomcc
Copy link
Contributor Author

thomcc commented Jan 5, 2023

Note: cargo-tarpaulin only provides binaries for x86_64-unknown-linux-gnu, and the tarballs have fairly idiosyncratic names. It's plausible that will have to change if cargo-tarpaulin ever provides other binaries, but at the moment it does not, so who knows.

P.S. cargo-quickinstall provides builds for x86_64-pc-windows-msvc and x86_64-apple-darwin.

Oh, hm. I'm noticing now that the GH releases for tarpaulin are behind a few versions too -- only covering up to 0.22.0 (latest is 0.23.1)... A bit unsure what to do here.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jan 5, 2023

Note: cargo-tarpaulin only provides binaries for x86_64-unknown-linux-gnu, and the tarballs have fairly idiosyncratic names. It's plausible that will have to change if cargo-tarpaulin ever provides other binaries, but at the moment it does not, so who knows.

P.S. cargo-quickinstall provides builds for x86_64-pc-windows-msvc and x86_64-apple-darwin.

Oh, hm. I'm noticing now that the GH releases for tarpaulin are behind a few versions too -- only covering up to 0.22.0 (latest is 0.23.1)... A bit unsure what to do here.

You could use quickinstall builds instead, which provides the latest v0.23.1

The name is also very predictable and I think it can run on old glibc.

Though I recommend use them through cargo-binstall, which will also report stats back to quickinstall.

@thomcc
Copy link
Contributor Author

thomcc commented Jan 5, 2023

You could use quickinstall builds instead, which provides the latest v0.23.1

Hm, I'm unsure how to set that up in the manifest templates, if that's what you're suggesting.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jan 5, 2023

You could use quickinstall builds instead, which provides the latest v0.23.1

Hm, I'm unsure how to set that up in the manifest templates, if that's what you're suggesting.

quickinstall url for cargo-tarpaulin goes like this:

https://github.com/cargo-bins/cargo-quickinstall/releases/download/cargo-tarpaulin-{version}-{target}/cargo-tarpaulin-{version}-{target}.tar.gz

@thomcc
Copy link
Contributor Author

thomcc commented Jan 5, 2023

https://github.com/cargo-bins/cargo-quickinstall/releases/download/cargo-tarpaulin-{version}-{target}/cargo-tarpaulin-{version}-{target}.tar.gz

I see. I think I need to extend tools/codegen to support this.

@thomcc
Copy link
Contributor Author

thomcc commented Jan 5, 2023

Ah, wait, maybe I'm being thick -- I see now that install-action already will automatically use cargo-binstall. I didn't realize this -- was your point that I should probably just drop this PR and go that route?

(I don't love that -- I think direct support would be better, but also I don't know how much more time I feel like spending on this...)

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jan 5, 2023

I see. I think I need to extend tools/codegen to support this.

Nope, this is just the same as you would do otherwise.
It's just a github release artifact.

Ah, wait, maybe I'm being thick -- I see now that install-action already will automatically use cargo-binstall. I didn't realize this -- was your point that I should probably just drop this PR and go that route?

That's also feasible since cargo-binstall can already install cargo-tarpaulin with no problem.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jan 5, 2023

(I don't love that -- I think direct support would be better, but also I don't know how much more time I feel like spending on this...)

I would say that if you don't have enough time, simply drop this and fallback to cargo-binstall.

IMHO cargo-binstall is quite good at its job and now cargo-quickinstall is also managed by us cargo-bins.

@thomcc
Copy link
Contributor Author

thomcc commented Jan 5, 2023

I'm just going to go with grcov for now, I think.

@thomcc thomcc closed this Jan 5, 2023
@taiki-e
Copy link
Owner

taiki-e commented Jan 5, 2023

I think the best way here would be to help them fix their release workflow and add linux-musl/macos/windows support (xd009642/tarpaulin#1130). (If they are fine with using my upload-rust-binary-action action (used by cargo-llvm-cov, tokio-console, nextest, etc.) then I think it should be easy to do.)


I see. I think I need to extend tools/codegen to support this.

Nope, this is just the same as you would do otherwise. It's just a github release artifact.

I think this need to at least add support for tag suffixes to codegen. Currently only tag prefixes are supported.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jan 5, 2023

I think this need to at least add support for tag suffixes to codegen. Currently only tag prefixes are supported.

Oops you are right, I didn't know this.

@thomcc
Copy link
Contributor Author

thomcc commented Jan 5, 2023

I think this need to at least add support for tag suffixes to codegen. Currently only tag prefixes are supported.

Yeah, and solving this is kind of annoying because the suffix isn't static the way the prefixes are -- it's the dynamic target name. Solving this required more thought than I had time for, so I punted.

I think the best way here would be to help them fix their release workflow and add linux-musl/macos/windows support (xd009642/tarpaulin#1130).

I happened to chat a bit with @xd009642 on Discord while writing this PR and I think they're open to this.

@orhun
Copy link
Contributor

orhun commented Jan 30, 2023

Now that cargo-tarpaulin has a proper release workflow, can we reconsider this?

@taiki-e
Copy link
Owner

taiki-e commented Feb 6, 2023

Yeah, I would accept a PR to support cargo-tarpaulin.

@orhun orhun mentioned this pull request Feb 6, 2023
@taiki-e
Copy link
Owner

taiki-e commented Feb 11, 2023

Added in 2.4.0 (#65).

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.

4 participants