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

[libsrtp] Add Exported CMake Targets for LibSRTP #20720

Merged
merged 5 commits into from
Feb 17, 2022

Conversation

SE2Dev
Copy link
Contributor

@SE2Dev SE2Dev commented Oct 13, 2021

Describe the pull request

This pull request adds libsrtp-config.cmake to the libsrtp port, which allows users to do the following in CMake:

find_package(libsrtp REQUIRED)
# ...
target_link_libraries(main PRIVATE LibSRTP::srtp2)

Additionally, I've updated the deprecated helper function usage for the libsrtp port.

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@SE2Dev SE2Dev marked this pull request as ready for review October 13, 2021 23:05
@NancyLi1013 NancyLi1013 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Oct 14, 2021
ports/libsrtp/vcpkg.json Show resolved Hide resolved
ports/libsrtp/libsrtp-config.cmake Outdated Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added the depends:upstream-changes Waiting on a change to the upstream project label Oct 14, 2021
@NancyLi1013 NancyLi1013 changed the title Add Exported CMake Targets for LibSRTP [libsrtp] Add Exported CMake Targets for LibSRTP Oct 14, 2021
@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013
Copy link
Contributor

Waiting for the response form upstream . cisco/libsrtp#573

@SE2Dev
Copy link
Contributor Author

SE2Dev commented Oct 15, 2021

I should point out that at least part of this pull request will no longer be necessary if the changes to upstream are accepted. If that happens, I'll update this request and #20722 accordingly.

@PhoebeHui PhoebeHui assigned LilyWangLL and unassigned NancyLi1013 Nov 22, 2021
@LilyWangLL LilyWangLL removed the depends:upstream-changes Waiting on a change to the upstream project label Feb 11, 2022
@LilyWangLL
Copy link
Contributor

@SE2Dev The upstream PR has been merged. Could you please merge fix on upstream to this PR.

@SE2Dev
Copy link
Contributor Author

SE2Dev commented Feb 14, 2022

@LilyWangLL Yes, although there's still no actual LibSRTP release that includes the merged changes. Should I selectively pull CMake files from a more current snapshot during the configuration process, or wait until there's a release that includes them?

@LilyWangLL
Copy link
Contributor

@LilyWangLL Yes, although there's still no actual LibSRTP release that includes the merged changes. Should I selectively pull CMake files from a more current snapshot during the configuration process, or wait until there's a release that includes them?

You can download patch file from https://patch-diff.githubusercontent.com/raw/cisco/libsrtp/pull/573.diff at runtime by vcpkg_download_distfile. Then add it to PATCHES in vcpkg_from_github.

@SE2Dev
Copy link
Contributor Author

SE2Dev commented Feb 15, 2022

I just pushed the requested changes. Since the path was never corrected in the original pull request before it was merged, I had to explicitly define the CONFIG_PATH for vcpkg_cmake_config_fixup.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libsrtp/vcpkg.json

Valid values for the license field can be found in the documentation

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I note upstream has already merged this.

@BillyONeal
Copy link
Member

I'm not sure why CLA bot is unhappy; have you signed a Microsoft CLA before?

@BillyONeal
Copy link
Member

It looks like CLA bot is wedged in many places :(

@SE2Dev
Copy link
Contributor Author

SE2Dev commented Feb 16, 2022

It seems to be complaining about the fact that there's no license field in vcpkg.json. LibSRTP has its own license file in its repo; assuming I'm reading this correctly, the license field should be set to null, correct?

@BillyONeal
Copy link
Member

That's a different bot; a suggestion but not a requirement. CLA is a requirement though. I have seen that wedged in other repos too; will keep you posted

@BillyONeal
Copy link
Member

Tried merging to see if that kicks the bot

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libsrtp/vcpkg.json

Valid values for the license field can be found in the documentation

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libsrtp/vcpkg.json

Valid values for the license field can be found in the documentation

@BillyONeal
Copy link
Member

@BillyONeal BillyONeal closed this Feb 17, 2022
@BillyONeal BillyONeal reopened this Feb 17, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libsrtp/vcpkg.json

Valid values for the license field can be found in the documentation

@BillyONeal
Copy link
Member

Closing and reopening the PR kicked the bot to be green :)

@BillyONeal BillyONeal added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Feb 17, 2022
@SE2Dev
Copy link
Contributor Author

SE2Dev commented Feb 17, 2022

Closing and reopening the PR kicked the bot to be green :)

Haha, nice! Apparently, I already signed the CLA at some point, so that shouldn't have been the problem.

@BillyONeal BillyONeal merged commit 09744c6 into microsoft:master Feb 17, 2022
@BillyONeal
Copy link
Member

Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants