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

chore(cargo-codspeed): bump cargo to 0.75 #31

Closed
wants to merge 1 commit into from
Closed

Conversation

art049
Copy link
Member

@art049 art049 commented Nov 20, 2023

Fixes #29

Copy link

codspeed-hq bot commented Nov 20, 2023

CodSpeed Performance Report

Merging #31 will not alter performance

Comparing bump-cargo (750e29a) with main (fc75139)

Summary

✅ 54 untouched benchmarks

@art049 art049 marked this pull request as draft November 20, 2023 12:41
@davidhewitt
Copy link
Contributor

@art049 is it necessary to build cargo into this package? Most other cargo subcommands I'm aware of use cargo as a CLI subprocess, e.g.

https://github.com/flamegraph-rs/flamegraph/blob/3c9d14643335db861521af1f66e1c2f9eb5aaf56/src/bin/cargo-flamegraph.rs#L89
https://github.com/taiki-e/cargo-llvm-cov/blob/0a65263bdafa3b72a26cf15c2e1c3410738df649/src/main.rs#L356

The downside of building cargo into the package like this is that it creates a mismatch between whatever Rust toolchain is installed and the bundled cargo version. This means that cargo-codspeed is currently missing the sparse registry and I worry it may have other bugs or subtle incompatibilities with the local Rust toolchain.

@art049
Copy link
Member Author

art049 commented Dec 5, 2023

@davidhewitt I think it's not necessary. At some point, it could have been interesting to "hot" replace the benchmark library and avoid using our compatibility layers. We haven't given it an in-depth look yet, so we're unsure if it would be possible.

But yes, definitely using the cargo CLI directly would make things simpler.

@art049 art049 closed this Apr 24, 2024
@art049
Copy link
Member Author

art049 commented Apr 24, 2024

Supersed by #43

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.

Update cargo to latest for cargo-codspeed?
2 participants