Skip to content

[liblsquic] Add new port #24310

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.

Sign up for GitHub

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

Jump to bottom
Merged
vicroms merged 4 commits into from
Jul 13, 2022
Merged

[liblsquic] Add new port #24310

vicroms merged 4 commits into from
Jul 13, 2022

Conversation

@rpavlik
Copy link
Contributor

@rpavlik rpavlik commented Apr 21, 2022 •
edited
Loading

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
github-actions[bot]
github-actions bot approved these changes Apr 22, 2022
View reviewed changes
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

JackBoosY
JackBoosY suggested changes Apr 22, 2022
View reviewed changes
@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 rpavlik requested a review from JackBoosY April 22, 2022 15:24
@rpavlik rpavlik mentioned this pull request Apr 22, 2022
Open
@rpavlik
Copy link
Contributor Author

rpavlik commented Apr 22, 2022

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

JackBoosY
JackBoosY suggested changes Apr 24, 2022
View reviewed changes
@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:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Apr 26, 2022
dg0yt
dg0yt reviewed Apr 26, 2022
View reviewed changes
BillyONeal
BillyONeal reviewed Apr 27, 2022
View reviewed changes
@rpavlik
Copy link
Contributor Author

rpavlik commented Jun 20, 2022 •
edited
Loading

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 )

@rpavlik rpavlik mentioned this pull request Jun 20, 2022
Merged
github-actions[bot]
github-actions bot previously approved these changes Jun 20, 2022
View reviewed changes
JackBoosY
JackBoosY previously approved these changes Jun 21, 2022
View reviewed changes
@JackBoosY
Copy link
Contributor

JackBoosY commented Jun 21, 2022

Testing this port locally.

@JackBoosY
Copy link
Contributor

JackBoosY commented Jun 21, 2022

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

@JackBoosY JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Jun 21, 2022
Thomas1664
Thomas1664 reviewed Jun 22, 2022
View reviewed changes
@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
@rpavlik rpavlik dismissed stale reviews from JackBoosY and github-actions[bot] via aaf0ab2 July 12, 2022 20:13
github-actions[bot]
github-actions bot previously approved these changes Jul 12, 2022
View reviewed changes
@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

Reviewers

@github-actions github-actions[bot] github-actions[bot] approved these changes

@BillyONeal BillyONeal Awaiting requested review from BillyONeal

@PhoebeHui PhoebeHui Awaiting requested review from PhoebeHui

+4 more reviewers

@dg0yt dg0yt dg0yt left review comments

@Thomas1664 Thomas1664 Thomas1664 left review comments

@JackBoosY JackBoosY JackBoosY left review comments

@raghavthind2005 raghavthind2005 raghavthind2005 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Adela0814 Adela0814

Labels

category:new-port The issue is requesting a new library to be added; consider making a PR! info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@rpavlik @BillyONeal @PhoebeHui @JackBoosY @dg0yt @Thomas1664 @raghavthind2005 @vicroms @dan-shaw @Adela0814