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

C++: Update libsrtp #1313

Closed
wants to merge 2 commits into from
Closed

C++: Update libsrtp #1313

wants to merge 2 commits into from

Conversation

jmillan
Copy link
Member

@jmillan jmillan commented Jan 15, 2024

Comes with performance enhancements on stream list traversal cisco/libsrtp#674

Using the pre-v3 branch which contains little API enhancements.

Comment on lines +2 to +5
directory = libsrtp-37d88ecac103f4e2a5336099c64b155ba4ca3b0c
source_url = https://github.com/versatica/libsrtp/archive/37d88ecac103f4e2a5336099c64b155ba4ca3b0c.zip
source_filename = libsrtp-37d88ecac103f4e2a5336099c64b155ba4ca3b0c.zip
source_hash = edf8b2c7a27ed6b6aac64c8d6a165379d3a0042bd420706b4a1825acc182441d
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why pulling from a fork rather than upstream?

Also FYI GitHub doesn't guarantee the hash of ZIP files from random revisions to not change over time. It is only guaranteed for releases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why pulling from a fork rather than upstream?

This is a mistake, I thought I was taking it from libsrtp repo directly.

Also FYI GitHub doesn't guarantee the hash of ZIP files from random revisions to not change over time. It is only guaranteed for releases.

That's right.., libsrtp version 3 is to be released sometime this year.., I'll ask for the timeline of the 2.5 release. I may create a PR for the enhancement to be included in 2.5.

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we create a GH relaase in our libsrtp fork and point to it in the wrap file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be a solution indeed

Copy link
Member Author

@jmillan jmillan Jan 16, 2024

Choose a reason for hiding this comment

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

Also FYI GitHub doesn't guarantee the hash of ZIP files from random revisions to not change over time. It is only guaranteed for releases.

I can't read where it says that a zip file for a certain commit hash may change. Indeed the documentation says it won't.

https://docs.github.com/en/repositories/working-with-files/using-files/downloading-source-code-archives#stability-of-source-code-archives

Copy link
Collaborator

Choose a reason for hiding this comment

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

An archive of a commit ID will always have the same file contents whenever it's requested, assuming the commit ID is still in the repository and the repository's name has not changed.

Only contents because:

The exact compression settings used to generate a zipball or tarball may change over time. The extracted contents won't change if the branch or tag doesn't change, but the outer compressed archive may have a different byte layout. GitHub will give at least six months' notice before changing compression settings.

Copy link
Member

Choose a reason for hiding this comment

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

Over a year ago GH changed the compression settings and half of the projects (including mediasoup) got into problems, so this is a real thing. However GH took the problem into consideration hence that new policy. Probably we are good whatever we do here but the only reliable way to go is by having a GH release.

Copy link
Collaborator

Choose a reason for hiding this comment

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

They have a note about 6 months warning, but I can imagine downstream projects might rely in older mediasoup releases that should ideally continue to compile in the future too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, I'll do a release from our fork then as suggested 👍

@jmillan
Copy link
Member Author

jmillan commented Jan 15, 2024

I'm closing this PR for now.

@jmillan jmillan closed this Jan 15, 2024
@jmillan jmillan deleted the update-libsrtp branch January 16, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants