-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[luasec] linux and macos support #37365
Conversation
Port |
Done. The remaining changes are the upstream move to lunarmodules and support for unix (linux/macos) targets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Port install passed with following triplets:
x64-windows
Why? |
@WangWeiLin-MV could you and @dg0yt come to a consensus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly not staticcrt
through the supports
field.
https://learn.microsoft.com/en-us/vcpkg/reference/vcpkg-json#platform-expression
It seems like there's either a supports change broken or a ci.baseline.txt change missing since nothing here will turn on linux and macos testing. |
@BillyONeal it's blocked by luasocket. I have changes to enable mac and linux on luasocket to merge after. Specifically #37344 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Port install passed with following triplets:
- x64-windows
It seems like luasocket has to go first then? I'm uncomfortable merging this change which claims to fix mac and linux when we don't have a build showing that it actually works on mac or linux |
(To be clear, that means: Go back to #37344 and fix luasocket. In the PR that fixes luasocket, mark luasec as failing with a 'supports' clause in vcpkg.json. After that merges, in this PR, remove that supports clause, so there is record in the PR that claims to fix it that it was actually fixed) |
if(WIN32) | ||
set(PLATFORM_LIBRARIES ws2_32) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this part needed after #37344 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the winsock dependency remains.
A little back story here in case anyone has other ideas how to approach it. luasocket existed first, primarily as a lua model. As such it exports (on Windows) only the symbol required to be loaded by lua. luasec was created by another person to add tls support to luasocket. They did this by adding another module for lua but they wanted to reuse code from luasocket. Their approach was to copy the luasocket code into luasec. It is possible (desirable for some) to static link lua modules, but in this case the symbols clash, so that's not possible with a naive approach (unless you are careful about versions). There have been a few discussions about it, but nothing has come from them that I'm aware. lunarmodules/luasec#145 Packaging luasocket and luasec together solves these problems, but as they are separate projects with separate versions and release schedules that doesn't quite work. |
Also change the upstream repo to lunarmodules since it was moved.
@WangWeiLin-MV Are we waiting for anything on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Port install passed with following triplets:
- x64-linux
- x64-osx
- x64-windows
- x64-windows-static-md
SHA512s are updated for each updated download.Any fixed CI baseline entries are removed from that file.Any patches that are no longer applied are deleted from the port's directory../vcpkg x-add-version --all
and committing the result.