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

[luarocks] fix compiler issues on CLANG64 and CLANGARM64, and update to 3.11.1 #22975

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

luau-project
Copy link
Contributor

Warning

It is a long PR. Two issues are reproduced, and the working fix is shown.

Description

Today, I found that the current package mingw-w64-lua-luarocks has two independent issues:

  1. On CLANG64 and CLANGARM64 environments, luarocks is built with a configuration to fallback to gcc as the default C compiler, which causes Lua modules written in C fail to build / install;
  2. luarocks fails to uninstall luafilesystem properly after a previous lfs package installation.

Note

Issue 2 has already been reported upstream luarocks/luarocks#1428, and fixed at luarocks/luarocks#1616. However, it still persists here without a proper fix.

Of lesser importance, MSYS2 is running version 3.9.2, while the latest luarocks release is 3.11.1.

Tip

I verified (cloned the branch and installed) that #20461 updates to 3.11.1, but the issues described here still persists with the solution provided there. So, I would like to invite @sewbacca and @Biswa96 to review this PR.

Table of Contents

  • Reproduction of Issue 1
  • Reproduction of Issue 2
  • After the fix

Reproduction of Issue 1

  1. Close any MSYS2 shell and launch a fresh CLANG64 shell

Important

  • CC environment variable must not be set
  • Make sure that luafilesystem is not installed for the current environment (either by luarocks, or by mingw-w64-clang-x86_64-lua-luafilesystem).
  1. Install luarocks:

    pacman -S ${MINGW_PACKAGE_PREFIX}-lua-luarocks
  2. Try to install any Lua package that has components written in C. For instance, luafilesystem is enough for the test:

    luarocks install luafilesystem

    The resulting output should be like this:

    issue-compiler

  3. Leave the shell opened and untouched for the reproduction of the issue 2.

Reproduction of Issue 2

Note

We are going to continue with the shell left opened from the issue 1.

  1. The issue 1 can be remedied by setting the CC environment variable, because luarocks will use it to build the C parts of a Lua module. So, set CC environment variable to the correct compiler:

    export CC=cc
  2. Try again to install luafilesystem (it should succeed):

    luarocks install luafilesystem
  3. List luarocks and check that luafilesystem was installed properly:

    luarocks list
  4. Remove luafilesystem through luarocks:

    luarocks remove luafilesystem
  5. Check that luarocks said to have removed luafilesystem properly, but did not:

    lua -e "print(require 'lfs')"

Note

lfs.dll can still be found at /clang64/lib/lua/5.4/lfs.dll

  1. Go ahead and remove it manually:

    rm ${MINGW_PREFIX}/lib/lua/5.4/lfs.dll

    issue-lfs

After the fix

Initial setup

  1. Close any MSYS2 shell and launch a fresh CLANG64 shell

Note

Ideally, CC environment variable should not be set. Remember that issue 1 can be bypassed by setting CC.

  1. Uninstall luarocks in case it is already installed, and also make sure that luafilesystem is not installed for the current environment (either by luarocks, or by mingw-w64-clang-x86_64-lua-luafilesystem);
  2. Download GitHub artifacts from the build and install them.

Check that issues 1 and 2 were fixed

  1. Install luafilesystem:

    luarocks install luafilesystem
  2. List luarocks and check that luafilesystem was installed properly:

    luarocks list
  3. Remove luafilesystem through luarocks:

    luarocks remove luafilesystem
  4. Check that luarocks removed luafilesystem properly, and it cannot find lfs.dll anymore

    lua -e "print(require 'lfs')"

    final-result

@luau-project
Copy link
Contributor Author

hello @hmartinez82

No, I didn't consider to upstream these changes, because most of them are MSYS2-specific, and would most likely be rejected.

Even though it looks like I did a lot of changes, no, I did not. If you browse the Files changes tab using the Diff view in Split mode, you can see that I basically replayed the changes on the patch file https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-lua-luarocks/0001-luarocks_msys2_mingw_w64.patch to luarocks 3.11.1 source code. However, like the comment #20461 (comment) said, that part had to be removed from the patch because the source code 3.11.1 changed.

The "original" part that I added (and is specific to MSYS2 environments) were:

  • to override the default CC and LD variables to cc in order to solve the discussed issue 1 on the main post;
  • I also added the code below to the config file to solve the discussed issue 2.
   if platforms.windows and (platforms.msys2_mingw_64 or (platforms.msys and platforms.mingw)) then                                                          
      defaults.fs_use_modules = false    
   end

Now, I realize that I shoud have opened two different issues and referenced them here, rather than reporting and fixing them directly in this PR.

Maybe @osch , who seems to be the last contributor to update luarocks, would like to join this discussion.

@osch
Copy link
Contributor

osch commented Feb 1, 2025

@luau-project how can I help here?

From my experience it is very difficult to get a merge request in msys2 for luarocks package, since no one from the msys2 team seems to be interested.

For example my merge request #12002 from Jul 2022, a very simple bugfixing, was ignored until I had luck and got this fixed in the merge request #20166 from Feb 2024. So from 2022 to 2024 the luarocks package in msys2 was broken (i.e. unusable without clumsy workarounds, see #12002 (comment)) on a fresh msys2 installed.

This demotivates me to put any effort in msys2 packages.

@luau-project
Copy link
Contributor Author

@luau-project how can I help here?

@osch

In my view, you could be extremely helpful just by asserting (or not) that:

  • in the current state (LuaRocks 3.9.2 stored on remote msys2 repo), you can reproduce issues 1 and 2 described on main post;
  • after the fix (either building yourself by applying the patch from this PR, or installing from the built GitHub artifacts), whether the proposed changes would fix issues 1 and 2 or not.

It would work as an informal review for a msys2 team member willing to take a decision (merge/reject/propose changes), even if such member does not have a strong Lua background, as I suspect it is the case.

@osch
Copy link
Contributor

osch commented Feb 1, 2025

a msys2 team member willing to take a decision

After my experience I doubt that there are such team members.

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.

2 participants