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

Add support for MSYS2 + Mingw-w64 #1231

Merged
merged 1 commit into from Oct 28, 2020

Conversation

kou
Copy link
Contributor

@kou kou commented Oct 8, 2020

Currently, LuaRocks supports:

  • (a) Lua interpreters built for MSYS2 (Lua interpreters depend on
    msys-2.0.dll).
    (the "msys" platform)

  • (b) Lua interpreters built by MinGW (Lua interpreters don't depend
    on msys-2.0.dll).
    (the "mingw" platform)

This change adds support for (c) Lua interpreters built as native
Windows application by MSYS2 + Mingw-w64 (Lua interpreters don't
depend on msys-2.0.dll). (the "msys2_mingw_w64" platform)

Here are differences between (a), (b) and (c):

  • (a) can't work without MSYS2 (msys-2.0.dll)
  • (b) can work without MSYS2
  • (c) can work without MSYS2 but is generally used with MSYS2
    because MSYS2 provides packages of useful libraries such as libxml2.

This change assumes that users use (c) with MSYS2. But this change
still uses win32/tools provided by LuaRocks not MSYS2.

MSYS2 has LuaRocks package:
https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-lua-luarocks

It applies a patch to support (c). If this change is merged into
LuaRocks, MSYS2 doesn't need to have a patch for LuaRocks.

Currently, LuaRocks supports:

  * (a) Lua interpreters built for MSYS2 (Lua interpreters depend on
    msys-2.0.dll).
    (the "msys" platform)

  * (b) Lua interpreters built by MinGW (Lua interpreters don't depend
    on msys-2.0.dll).
    (the "mingw" platform)

This change adds support for (c) Lua interpreters built as native
Windows application by MSYS2 + Mingw-w64 (Lua interpreters don't
depend on msys-2.0.dll). (the "msys2_mingw_w64" platform)

Here are differences between (a), (b) and (c):

  * (a) can't work without MSYS2 (msys-2.0.dll)
  * (b) can work without MSYS2
  * (c) can work without MSYS2 but is generally used with MSYS2
    because MSYS2 provides packages of useful libraries such as libxml2.

This change assumes that users use (c) with MSYS2. But this change
still uses win32/tools provided by LuaRocks not MSYS2.

MSYS2 has LuaRocks package:
https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-lua-luarocks

It applies a patch to support (c). If this change is merged into
LuaRocks, MSYS2 doesn't need to have a patch for LuaRocks.
@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #1231 into master will decrease coverage by 20.62%.
The diff coverage is 14.81%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1231       +/-   ##
===========================================
- Coverage   85.49%   64.87%   -20.63%     
===========================================
  Files          94      259      +165     
  Lines       11472    32258    +20786     
===========================================
+ Hits         9808    20926    +11118     
- Misses       1664    11332     +9668     
Impacted Files Coverage Δ
src/luarocks/core/cfg.lua 76.06% <14.81%> (-12.14%) ⬇️
src/luarocks/admin/cmd/remove.lua 31.57% <0.00%> (-61.41%) ⬇️
src/luarocks/fs/lua.lua 25.25% <0.00%> (-60.44%) ⬇️
src/luarocks/admin/cmd/add.lua 40.00% <0.00%> (-52.95%) ⬇️
src/luarocks/core/sysdetect.lua 41.33% <0.00%> (-33.67%) ⬇️
src/luarocks/admin/cmd/refresh_cache.lua 61.11% <0.00%> (-33.34%) ⬇️
src/luarocks/admin/cache.lua 58.49% <0.00%> (-24.53%) ⬇️
src/luarocks/upload/api.lua 63.84% <0.00%> (-12.43%) ⬇️
src/luarocks/build/builtin.lua 90.49% <0.00%> (-4.98%) ⬇️
... and 183 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a356aa...d2ba78b. Read the comment docs.

@kou
Copy link
Contributor Author

kou commented Oct 23, 2020

@hishamhm Could you review this?

@hishamhm hishamhm merged commit d2c3b57 into luarocks:master Oct 28, 2020
2 checks passed
@hishamhm
Copy link
Member

Merged, thank you! Appreciated replacing the io.open test with the MSYSTEM environment variable.

Adding this link here for future reference:
https://stackoverflow.com/questions/37460073/msys-vs-mingw-internal-environment-variables

@kou kou deleted the msys2-mingw-w64-platform branch October 29, 2020 00:23
@kou
Copy link
Contributor Author

kou commented Oct 29, 2020

Thanks!
I'll work on adding CI jobs for MSYS2 + Mingw-w64 as the next step.

@hishamhm
Copy link
Member

@kou Great! That would be excellent for preventing regressions!

@SlySven
Copy link

SlySven commented Nov 21, 2020

💥 Um, you guys... are you aware that when using Mingw-w64 you could be working in a 32-Bit windows shell OR a 64-bit one - you get three environments/shells:

  • the MSYS2 one - typically for things that are command line or at least not GUI and which provides an environment for GNU autotools to run
  • a Mingw64 one - that can only be used on a 64-Bit system (AFAICT) - which uses native 64-bit Windows calls for typically GUI applications
  • a Mingw32 one - that can run inside the WindowsOnWindows64 (WoW64) sub-system on a 64-Bit system (or could have been used on a 32-Bit system up to May this year +) which uses 32-bit Windows calls (that get thunked by WoW64 on 64-bit system) also for typically GUI applications

The importance thing is that the latter two are segregated and if you are compiling things on a 64-Bit system for both 64-bit AND 32-bit environments you need to maintain 2 separate sets of luarocks - MSYS2/Mingw-w64 now does that a bit better since I fixed a problem in that (see: msys2/MINGW-packages#5928 --> msys2/MINGW-packages#6580).

As I mention in the issue in that first URL, whilst installing rocks "system-wide" (the default) this is handled automatically because the files are placed in the appropriate (C:\msys64\mingw64 / C:\msys64\mingw32) sub-directories. However per-user ones (using the --local option to most luarocks commands) will have problems by default as they both - without some hacking of the corresponding C:\msys64\mingw64\etc\luarocks\config-5.1.lua and C:\msys32\mingw64\etc\luarocks\config-5.1.lua files so that they have different paths for the value against the root key for the sub-tree entry in the rock_trees that has the key user - will put their compiled/generated binary/lua files in the SAME location and the binary (C .dll files) one are NOT going to be the same. For my own use I have changed my C:\msys64\mingw64\etc\luarocks\config-5.1.lua to be:

 rocks_trees = {
-   { name = [[user]], root = home..[[/.luarocks]] },
+   { name = [[user]], root = home..[[/.luarocks-x64]] },
    { name = [[system]], root = [[C:/msys64/mingw64]] }
 }

which works but other users will also need to do something like this should they install both mingw-w64-i686-lua51-luarocks AND mingw-w64-x86_64-lua51-luarocks...

+ - MSYS2 dropped support for installation on a 32-Bit system earlier this year but one can still build applications for it - like the project I code for will do once we get the kinks out.

@kou
Copy link
Contributor Author

kou commented Nov 21, 2020

You say about LuaRocks configuration files not LuaRocks' internal platform configuration, right?
It's out of scope of this pull request.
We should handle it in msys2/MINGW-packages.

@SlySven
Copy link

SlySven commented Nov 26, 2020

Well might it also be an issue on other OSes when the user is targetting both 32 and 64 -bit platforms. Fair enough it is only going to be an issue for those who are effectively cross-compiling one (probably the 32-bit one) and native compiling the other on the other (probably 64-bit one).

In the case of only a single install of one of those bitnesses it would probably require considering a bit-specific root = home..[[/.luarocks-x64]] or root = home..[[/.luarocks-x32]] first and only dropping back to a generic root = home..[[/.luarocks]] if the particular former one is not found - it seems to me though that this is better handled inside luarocks as then it can be standardised across platforms...

@kou
Copy link
Contributor Author

kou commented Nov 26, 2020

Once again, you say about LuaRocks configuration files not LuaRocks' internal platform configuration, right?
If it's right and you think that we should improve LuaRocks itself, how about creating a new issue or pull request instead of reusing this pull request?
Because this pull request isn't for LuaRocks configuration files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants