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

update dependendies (ahash, quick-xml) #258

Merged
merged 8 commits into from Oct 30, 2022

Conversation

crepererum
Copy link
Contributor

Hey 👋

We're using your nice crate over at https://github.com/influxdata/influxdb_iox (indirectly via pprof) and I've seen that some of our the dependencies that inferno pulls are slightly outdated and hence get compiled twice. So I've decided to update them. 🙂

jonhoo
jonhoo previously approved these changes Sep 17, 2022
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Excellent, thanks for taking the time!

@jonhoo
Copy link
Owner

jonhoo commented Sep 17, 2022

Unfortunately, it looks like some of the CI jobs fail 🤔 I'm guessing some of the formatting from quick-xml may have changed.

Separately, I noticed that criterion has also had a new release — would you mind including that in the PR?

@crepererum
Copy link
Contributor Author

crepererum commented Sep 19, 2022

Working on it (probably needs a few iterations because I cannot get the tests to run locally (the perf and dtrace tests fail all the time and I don't really know why).

Update: I found out why my local tests don't pass, git submodules FTW 😅

jonhoo
jonhoo previously approved these changes Oct 9, 2022
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

@codecov
Copy link

codecov bot commented Oct 9, 2022

Codecov Report

Base: 90.33% // Head: 90.29% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (bb01949) compared to base (3ab9716).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #258      +/-   ##
==========================================
- Coverage   90.33%   90.29%   -0.04%     
==========================================
  Files          19       19              
  Lines        4241     4226      -15     
==========================================
- Hits         3831     3816      -15     
  Misses        410      410              
Impacted Files Coverage Δ
src/flamegraph/color/mod.rs 86.56% <100.00%> (ø)
src/flamegraph/color/palettes.rs 99.04% <100.00%> (ø)
src/flamegraph/mod.rs 97.93% <100.00%> (-0.01%) ⬇️
src/flamegraph/svg.rs 97.75% <100.00%> (-0.13%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jonhoo
Copy link
Owner

jonhoo commented Oct 9, 2022

Looks like we have a couple of CI failures:

  • The minimal-versions one seems to be because we somehow now pull in itoa 0.4.0, which is super old. Not sure why. If you run cargo update -Zminimal-versions and then cargo tree -i itoa:0.4.0, you may get some pointers to what's happened. That said, I'm fine merging even if that one is red. It could be that merging from main will fix the problem for you.
  • The MSRV CI step (1.59.0) is because of a MSRV regression in quick-xml. It's been fixed upstream, but there hasn't been a release yet: Remove use of const fn, add CI tests for MSRV tafia/quick-xml#481 (comment). I'd actually like to hold off on merging this PR until that lands (at which point we should bump the quick-xml dependency to 0.25.1).
  • The check / stable /fmt CI step seems to complain about the lack of cargo fmt being applied to a line you changed — should be an easy fix!

@crepererum
Copy link
Contributor Author

This is now solely blocked by the quick-xml release.

@jonhoo
Copy link
Owner

jonhoo commented Oct 25, 2022

We should also do a bump of clap to major version 4, but I suppose it's no rush (and can be done in a separate PR).

@crepererum
Copy link
Contributor Author

Updated to quick-xml 0.26 -- which fixes the MSRV. Let's do clap in another PR, also because this would mean that we compile two version of clap (at least for testing) because criterion still uses V3.

@jonhoo jonhoo merged commit 7f14ad7 into jonhoo:main Oct 30, 2022
@jonhoo
Copy link
Owner

jonhoo commented Oct 30, 2022

Ah, forgot the changelog! Would you mind updating that too?

@crepererum crepererum mentioned this pull request Nov 1, 2022
@crepererum
Copy link
Contributor Author

Ah, forgot the changelog! Would you mind updating that too?

#273.

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.

None yet

2 participants