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

[liblsquic] Add new port #24310

Merged
merged 4 commits into from
Jul 13, 2022
Merged

[liblsquic] Add new port #24310

merged 4 commits into from
Jul 13, 2022

Conversation

rpavlik
Copy link
Contributor

@rpavlik rpavlik commented Apr 21, 2022

Adds a port for lsquic, an implementation of several versions of QUIC and HTTP/3.

The thing I have the biggest question about is the submodules: I tried to follow the technique I saw in a few other ports, but not sure if it's the preferred way.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

I have tested Linux and Windows, have not updated the CI baseline.

Yes - patches have already been upstreamed and dropped.

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

yes

@rpavlik rpavlik changed the title Add liblsquic port. [liblsquic] Add new port Apr 21, 2022
@rpavlik rpavlik marked this pull request as ready for review April 21, 2022 18:27
@Adela0814 Adela0814 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Apr 22, 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/liblsquic/vcpkg.json

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

ports/liblsquic/export_targets.patch Outdated Show resolved Hide resolved
@rpavlik
Copy link
Contributor Author

rpavlik commented Apr 22, 2022

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

I can add this

@rpavlik rpavlik force-pushed the lsquic branch 2 times, most recently from ae9a32f to d7feb53 Compare April 22, 2022 14:19
@rpavlik
Copy link
Contributor Author

rpavlik commented Apr 22, 2022

OK, I have added the license field, removed the unneeded patch, and the install patch has been submitted upstream. Not sure if there's anything I need to do to explicit update the CI baseline, since it appears to build everywhere just fine.

@rpavlik
Copy link
Contributor Author

rpavlik commented Apr 22, 2022

Oh, hmm, I missed this detail in the maintainer guide at first: "Libraries that don't provide a .def file and do not use __declspec() declarations simply do not support shared builds for Windows and should be marked as such with vcpkg_check_linkage(ONLY_STATIC_LIBRARY)." I think it only provides a C api, do I still need to add this line? It's been apparently working OK with shared builds on Windows here...

@rpavlik
Copy link
Contributor Author

rpavlik commented Apr 22, 2022

OK I added vcpkg_check_linkage(ONLY_STATIC_LIBRARY) on Windows in the portfile.

ports/liblsquic/portfile.cmake Outdated Show resolved Hide resolved
ports/liblsquic/portfile.cmake Outdated Show resolved Hide resolved
@rpavlik
Copy link
Contributor Author

rpavlik commented Apr 25, 2022

OK, revised as requested.

@rpavlik rpavlik requested a review from JackBoosY April 25, 2022 15:05
@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Apr 26, 2022
@rpavlik
Copy link
Contributor Author

rpavlik commented Jun 20, 2022

OK, works on x64-windows, x64-windows-static, and x64-linux.

Had to put in a file install because it was yanked in a bulk release commit which I had missed. ( PR to restore upstream litespeedtech/lsquic#389 )

This is ready to review and merge. I'm following up on the 32-bit stuff (and the one required cast litespeedtech/lsquic#388 ) and the PERL/PERL_EXECUTABLE (litespeedtech/lsquic#387 litespeedtech/ls-qpack#42 )

github-actions[bot]
github-actions bot previously approved these changes Jun 20, 2022
JackBoosY
JackBoosY previously approved these changes Jun 21, 2022
@JackBoosY
Copy link
Contributor

Testing this port locally.

@JackBoosY
Copy link
Contributor

Already tested successfully on x64-windows, x64-windows-static, x64-linux.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 21, 2022
@dan-shaw dan-shaw added the depends:different-pr This PR or Issue depends on a PR which has been filed label Jun 23, 2022
@vicroms vicroms changed the title [liblsquic] Add new port [liblsquic] Add new port (waiting for #25239 vcpkg_install_copyright) Jul 11, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 12, 2022
@vicroms vicroms removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Jul 13, 2022
@vicroms vicroms changed the title [liblsquic] Add new port (waiting for #25239 vcpkg_install_copyright) [liblsquic] Add new port Jul 13, 2022
@vicroms vicroms merged commit 5f8189b into microsoft:master Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants