Skip to content

Windows: Build lua with 5.2 compatibility#12978

Merged
mattklein123 merged 2 commits intoenvoyproxy:masterfrom
greenhouse-org:windows-build-lua-5.2-compat
Sep 9, 2020
Merged

Windows: Build lua with 5.2 compatibility#12978
mattklein123 merged 2 commits intoenvoyproxy:masterfrom
greenhouse-org:windows-build-lua-5.2-compat

Conversation

@sunjayBhatia
Copy link
Member

Commit Message: Windows: Build lua with 5.2 compatibility
Additional Description: As per http://lua-users.org/wiki/MetatableEvents the __pairs metamethod is not available until lua 5.2, luajit/moonjit build lua 5.1. This define was set on Linux/Mac but not Windows.
Risk Level: Low
Testing: Enables 3 failing lua tests on Windows, checked locally on Windows
Docs Changes: N/A
Release Notes: N/A

As per http://lua-users.org/wiki/MetatableEvents the __pairs metamethod
is not available until lua 5.2, luajit/moonjit build lua 5.1. This
define was set on Linux/Mac but not Windows.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia
Copy link
Member Author

cc @envoyproxy/windows-dev

@setlocal
-@set LJCOMPILE=cl /nologo /c /O2 /W3 /D_CRT_SECURE_NO_DEPRECATE /D_CRT_STDIO_INLINE=__declspec(dllexport)__inline
+@set LJCOMPILE=cl /nologo /c /W3 /D_CRT_SECURE_NO_DEPRECATE /D_CRT_STDIO_INLINE=__declspec(dllexport)__inline
+@set LJCOMPILE=cl /nologo /c /W3 /D_CRT_SECURE_NO_DEPRECATE /D_CRT_STDIO_INLINE=__declspec(dllexport)__inline /DLUAJIT_ENABLE_LUA52COMPAT
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, just curious, why we remove /O2 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

/O2 placed there was being applied to to debug and release build modes, it was removed here to enable us to target it to just release mode and disable optimizations entirely in debug

cc @wrowe

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll find /O2 is still applied appropriately later in the script, @dio

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks, @wrowe.

@dio
Copy link
Member

dio commented Sep 7, 2020

@sunjayBhatia this looks good. However, we need to merge main to pick up a fix.

….2-compat

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
@wrowe
Copy link
Contributor

wrowe commented Sep 7, 2020

@dio done, thanks. (Sunjay is on PTO)

Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

LGTM

@mattklein123 mattklein123 merged commit 03d8c87 into envoyproxy:master Sep 9, 2020
@wrowe wrowe deleted the windows-build-lua-5.2-compat branch September 9, 2020 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants