search

JuliaPerf / PProf.jl

Export Julia profiles to the pprof format
MIT License
155 stars 18 forks source link

Migrate to ProtoBuf.jl 1.0 #65

Closed Drvi closed 2 years ago

Drvi commented 2 years ago

Recently, we published a new version of ProtoBuf.jl (discourse link), and this PR migrates to the new version.

Sadly, the new version doesn't (yet) support comment/docstring preservation, hopefully, it is ok to lose them for now.

NHDaly commented 2 years ago

Thanks @Drvi! The old version didn't support it either, since my PR was never merged: https://github.com/JuliaIO/ProtoBuf.jl/pull/128 😒

But we generated these from that PR. Anyway, yeah, totally fine to lose them πŸ‘ Hopefully that can be added back at some point though, because the pprof proto spec is MAD confusing! πŸ˜…

Thanks :)

codecov-commenter commented 2 years ago

Codecov Report

Merging #65 (ba01b7a) into master (d995401) will increase coverage by 0.44%. The diff coverage is 96.29%.

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   96.46%   96.90%   +0.44%     
==========================================
  Files           3        3              
  Lines         311      291      -20     
==========================================
- Hits          300      282      -18     
+ Misses         11        9       -2
Impacted Files Coverage Ξ”
src/Allocs.jl 93.15% <94.44%> (+1.90%) :arrow_up:
src/flamegraphs.jl 96.84% <94.73%> (-0.19%) :arrow_down:
src/PProf.jl 99.18% <100.00%> (-0.05%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

NHDaly commented 2 years ago

@Drvi: It looks like ProtoBuf.jl 1.0 doesn't support julia 1.3 anymore?

I think we can probably bump up our minimum supported julia version with this release. That seems reasonable to me. πŸ‘

Drvi commented 2 years ago

Thank you, @NHDaly! I tried locally on peakflops and seemed fine to me, but would be great if someone else could verify.

I just noticed that the julia 1.3 tests are failing because ProtoBuf targets 1.7+... is it ok to bump the minimum supported version here as well?

NHDaly commented 2 years ago

πŸ€” is it possible to keep our minimum supported julia version to be 1.6, since that's the LTS? Profiling is a fairly important thing, and I'd like to keep compatibility with 1.6+ if possible.. Is ProtoBuf.jl only 1.7+? Does it need to be 1.7+?

Drvi commented 2 years ago

That is a really good point, I think we can support 1.6 -- trying here https://github.com/JuliaIO/ProtoBuf.jl/pull/203

Drvi commented 2 years ago

Locally, all tests were passing for me on 1.6

Drvi commented 2 years ago

Ok, v1.0.1 of ProtoBuf should be in the general registry soon and with it, we should be able to support 1.6 here.

Drvi commented 2 years ago

Please feel free to merge if you are happy with the changes πŸ‘

NHDaly commented 2 years ago

amazing. Thanks for the quick turnaround on 1.6 support! ❀️ and thanks for the PR!