Skip to content

Update puffin and fix CI#94

Open
emilk wants to merge 13 commits intoaclysma:masterfrom
rerun-io:emilk/puffin-0.20
Open

Update puffin and fix CI#94
emilk wants to merge 13 commits intoaclysma:masterfrom
rerun-io:emilk/puffin-0.20

Conversation

@emilk
Copy link
Copy Markdown
Contributor

@emilk emilk commented Mar 18, 2026

  • Update MSRV to 1.92
  • Update CI to use 1.92
  • Update to puffin 0.20 (requires rustc 1.92)
  • Update all other dependencies
  • Add Cargo.lock to git (for reproducible builds)
  • Add rust-toolchain.toml

@emilk emilk changed the title Update to puffin 0.20 Update MSRV and all dependencies Mar 18, 2026
@aclysma
Copy link
Copy Markdown
Owner

aclysma commented Mar 18, 2026

A few comments:

  • while I see that general guidance for cargo.lock has been updated to be “check it in”, after looking at the rationale here I prefer not to have it checked in for library projects: fix: Change the defaults to always check-in Cargo.lock rust-lang/cargo#12382
  • This project has a strict no-copyleft requirement for dependencies, so MPL cannot be added to the allowed upstream licenses. I did not review the other licenses. Are they necessary?
  • Is the MSRV necessary to bump? It should only be bumped if absolutely necessary to compile with no backend enabled. Since we have several backends with their own dependency chains, our minimum MSRV would be overly constrained if we took the highest of all dependencies. Technically, raising MSRV is a breaking change that should include a major version bump, but practically this crate is not useful if we fragment which version people are using, so we can’t really bump the major version. Since we can’t bump the major version, as a compromise, I have been extremely conservative about MSRV.

@emilk
Copy link
Copy Markdown
Contributor Author

emilk commented Mar 18, 2026

Thanks for the feedback! I think we need 1.92 in order to check the crates on CI, but we should still be able to keep the MSRV on 1.60. Let me try!

@emilk emilk changed the title Update MSRV and all dependencies Update dependencies and fix CI Mar 18, 2026
@emilk emilk changed the title Update dependencies and fix CI Update puffin and fix CI Mar 18, 2026
@emilk
Copy link
Copy Markdown
Contributor Author

emilk commented Mar 18, 2026

I've confirmed that cargo +1.60.0 check -p profiling --no-default-features still works!

All your other concerns have been addressed too

@emilk
Copy link
Copy Markdown
Contributor Author

emilk commented Mar 18, 2026

All I really set out to do was update to puffin 0.20, but then I got a bit carried away… 😅

@emilk
Copy link
Copy Markdown
Contributor Author

emilk commented Mar 23, 2026

Would you like me to split this into smaller PRs?

@kocsis1david
Copy link
Copy Markdown

I switched to this PR's branch and had problems with dependecy versions being too strict, cargo couldn't select proper versions.

I changed this to 1:

https://github.com/aclysma/profiling/pull/94/changes#diff-7e076d27791f7ab0904587d54e71ab99b70d1ecf31e40f4aa259c0ead3541c7eR16

and also changed others to not contain patch versions and now it works.

@aclysma
Copy link
Copy Markdown
Owner

aclysma commented Mar 31, 2026

I'm sorry for the delay, I've been very busy professionally and I don't really use rust anymore on a day-to-day basis. I reviewed more carefully and I have a few questions:

  • Is rust-toolchain.toml pinning a particular version of rust? Why would we want that? Is this just for the benefit of tools like clippy or does it affect more than that? (The CI should check stable on all platforms, beta on one platform, and try compiling with the MSRV compiler. It should already be doing that. So I'm not sure what the benefit of rust-toolchain.toml would be, and I'm not sure this repo should be opinionated about the rust version aside from CI checking MSRV and latest stable.)
  • why do we have targets in cargo deny? Why is it necessary to list these?
  • MSRV is bumped in the ci.yml, but I think we discussed not bumping the MSRV
  • is the change in profiling-procmacros/src/lib.rs necessary? Does this raise MSRV?
  • The concern @kocsis1david raises seems valid as well

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.

3 participants