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

[luasec] linux and macos support #37365

Merged
merged 2 commits into from
Apr 24, 2024
Merged

Conversation

TheCycoONE
Copy link
Contributor

@TheCycoONE TheCycoONE commented Mar 11, 2024

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@TheCycoONE TheCycoONE marked this pull request as ready for review March 11, 2024 02:08
@WangWeiLin-MV WangWeiLin-MV added the category:port-update The issue is with a library, which is requesting update new revision label Mar 11, 2024
@WangWeiLin-MV
Copy link
Contributor

Port luasec version 1.3.2 has been merged in #37294 , please resolve conflicts and increase port-version for other remaining content.

@TheCycoONE
Copy link
Contributor Author

Done. The remaining changes are the upstream move to lunarmodules and support for unix (linux/macos) targets.

@TheCycoONE TheCycoONE changed the title Luasec bump [luasec] linux and macos support Mar 12, 2024
WangWeiLin-MV
WangWeiLin-MV previously approved these changes Mar 13, 2024
Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a 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

ports/luasec/vcpkg.json Outdated Show resolved Hide resolved
@WangWeiLin-MV WangWeiLin-MV marked this pull request as draft March 15, 2024 08:31
@dg0yt
Copy link
Contributor

dg0yt commented Mar 16, 2024

@WangWeiLin-MV WangWeiLin-MV marked this pull request as draft

Why?

@TheCycoONE
Copy link
Contributor Author

@WangWeiLin-MV could you and @dg0yt come to a consensus.

Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a 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

@TheCycoONE TheCycoONE marked this pull request as ready for review March 18, 2024 16:22
@BillyONeal
Copy link
Member

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.

@TheCycoONE
Copy link
Contributor Author

TheCycoONE commented Mar 19, 2024

@BillyONeal it's blocked by luasocket. I have changes to enable mac and linux on luasocket to merge after. Specifically #37344

WangWeiLin-MV
WangWeiLin-MV previously approved these changes Mar 19, 2024
Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a 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

@WangWeiLin-MV WangWeiLin-MV added the info:reviewed Pull Request changes follow basic guidelines label Mar 20, 2024
@BillyONeal
Copy link
Member

@BillyONeal it's blocked by luasocket. I have changes to enable mac and linux on luasocket to merge after. Specifically #37344

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

@BillyONeal BillyONeal marked this pull request as draft March 20, 2024 20:04
@BillyONeal
Copy link
Member

(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)

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Mar 20, 2024
@BillyONeal BillyONeal marked this pull request as ready for review March 25, 2024 23:02
@BillyONeal BillyONeal added the info:reviewed Pull Request changes follow basic guidelines label Mar 25, 2024
Comment on lines +14 to +16
if(WIN32)
set(PLATFORM_LIBRARIES ws2_32)
endif()
Copy link
Member

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 ?

Copy link
Contributor Author

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.

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Mar 25, 2024
@TheCycoONE
Copy link
Contributor Author

TheCycoONE commented Mar 26, 2024

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
lunarmodules/luasocket#346

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.
@TheCycoONE
Copy link
Contributor Author

@WangWeiLin-MV Are we waiting for anything on this?

Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV left a 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

@WangWeiLin-MV WangWeiLin-MV added the info:reviewed Pull Request changes follow basic guidelines label Apr 15, 2024
@JavierMatosD JavierMatosD merged commit 069695d into microsoft:master Apr 24, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants