Skip to content

Bump ring 0.17 in dev-deps, bump to latest#1528

Merged
djc merged 4 commits into
mainfrom
ring-0.17-bumps
Oct 10, 2023
Merged

Bump ring 0.17 in dev-deps, bump to latest#1528
djc merged 4 commits into
mainfrom
ring-0.17-bumps

Conversation

@djc

@djc djc commented Oct 9, 2023

Copy link
Copy Markdown
Member

Let's see what the benchmark results look like.

@djc
djc requested review from cpu and ctz October 9, 2023 08:10
@codecov

codecov Bot commented Oct 9, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1528 (bed29f9) into main (7edbfb9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1528   +/-   ##
=======================================
  Coverage   96.46%   96.46%           
=======================================
  Files          74       74           
  Lines       15113    15113           
=======================================
  Hits        14579    14579           
  Misses        534      534           

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

Comment thread examples/Cargo.toml Outdated
@ctz ctz changed the title Bump ring 0.27 in dev-deps, bump to latest Bump ring 0.17 in dev-deps, bump to latest Oct 9, 2023
@djc
djc force-pushed the ring-0.17-bumps branch from 9bed26b to bed29f9 Compare October 9, 2023 08:40
@djc

djc commented Oct 9, 2023

Copy link
Copy Markdown
Member Author

So it looks like we don't even get near reclaiming the performance losses from #1508, which is surprising?

@djc djc mentioned this pull request Oct 9, 2023
@aochagavia

aochagavia commented Oct 10, 2023

Copy link
Copy Markdown
Contributor

As discussed on Discord, it looks like there is a regression when GCC is used to compile ring's C code, but not when Clang is used. The bench runner is configured to use GCC, so it shows the regression.

I've tested using Clang 14 and there seem to be no regressions there (see here for the details).

Note for the future: we can use Clang in the CI instead of GCC by configuring the cc alias accordingly. However, that would require using a more recent version of valgrind, because versions prior to 3.20 choke on Clang's debuginfo.

@djc
djc added this pull request to the merge queue Oct 10, 2023
Merged via the queue into main with commit 21484e1 Oct 10, 2023
@djc
djc deleted the ring-0.17-bumps branch October 10, 2023 13:26
@MichaIng

MichaIng commented Oct 14, 2023

Copy link
Copy Markdown

This is highly welcome, also since ring 0.17 adds support for RISC-V. But builds still fail since ring 0.16 is still tried to be built in addition to 0.17: https://github.com/rustls/rustls/blob/ac10422/Cargo.lock#L773

I do not see any other create adding it as dependency, so can this duplicate+old ring entry be removed from the Cargo.lock?

@djc

djc commented Oct 14, 2023

Copy link
Copy Markdown
Member Author

Those are just dev-dependencies, it shouldn't affect anything if you just want to build the library.

@MichaIng

Copy link
Copy Markdown

I recognised just now that this change, and the one which moved ring from deps to dev-deps has not been released yet. I will test it with an 0.22 alpha version in our project.

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.

5 participants