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

Path 2: update rcgen to version 0.13.1 #5592

Closed
wants to merge 4 commits into from
Closed

Conversation

haurog
Copy link

@haurog haurog commented Sep 5, 2024

As described in #5590 this PR shows all the necessary changes for update path 2.
In this solution, rcgen and webrtc is updated to the newest version which makes substantial code changes necessary.

Notes & open questions

I do not know the code base well enough to make sure I did not break anything here. All the tests (minus doc test) work and I also built lighthouse which depends on libp2p2-tls and it works flawlessly, so I do not think I broke anything obvious.

I did not change the documentation which is why doctest tests fail.

Change checklist

I did not do the changes described below. I can do this once it is decided which update Path is the one to go.

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Hi, and thanks for this! I definitely prefer this path over Path1 as I had started the same procedure on #5448 but haven't yet had time to fix the tests, do you wanna take it over? that would be great!
Thanks!

@haurog
Copy link
Author

haurog commented Sep 5, 2024

That is awesome. I will definitely have a look at it and see if I can fix the test.

Copy link
Contributor

mergify bot commented Sep 24, 2024

This pull request has merge conflicts. Could you please resolve them @haurog? 🙏

@jxs
Copy link
Member

jxs commented Oct 4, 2024

going to close this one then

@jxs jxs closed this Oct 4, 2024
@kayabaNerve
Copy link
Contributor

Sorry for being out of the loop, yet why was this closed @jxs? rcgen wasn't updated from 0.11 so this remains a candidate. I'd guess the still open 0.12 PR is preferred yet I can't find any statement/documentation on that.

@dariusc93
Copy link
Member

Sorry for being out of the loop, yet why was this closed @jxs? rcgen wasn't updated from 0.11 so this remains a candidate. I'd guess the still open 0.12 PR is preferred yet I can't find any statement/documentation on that.

I believe this one was closed so it all can be done in #5591.

@kayabaNerve
Copy link
Contributor

That PR's last message was over a month ago, while this one was closed two weeks ago, hence my question what caused an action two weeks ago. If the answer is in fact that upgrading to 0.12 is preferred (my stated guess), I'd just appreciate confirmation and documented reasoning for it (as I'd assume upgrading to rcgen 0.13 would be generally preferred and jxs prior commented "I definitely prefer this path").

@jxs
Copy link
Member

jxs commented Oct 30, 2024

Hi @kayabaNerve, as referred above this was closed because #5591 was started first and @haurog stated that he was going to look into it as they mention on #5592 (comment).

Are you interested in taking it over?

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.

4 participants