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

feature: MSYS2 support on src/makefile #435

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

Conversation

luau-project
Copy link

Description

The changes in the src/makefile allows to build luasocket on MSYS2 shells with the compiler toolchain for each environment:

  • mingw32
  • mingw64
  • ucrt64
  • clang32
  • clang64
  • clangarm64

How to test the changes

Initial setup of tools (only once)

  1. Go to https://www.msys2.org/, download the installer and install MSYS2;
  2. Type ucrt64 on Windows start menu to open a MSYS2 shell for the ucrt64 environment listed above;
  3. Update core system packages
    pacman -Syuu
  4. Sync package database
    pacman -Syuu
  5. Install useful linux tools, git, a C compiler and Lua 5.1:
    pacman -S --needed base-devel git mingw-w64-ucrt-x86_64-cc mingw-w64-ucrt-x86_64-lua51
  6. Change to /tmp directory and clone my branch
    cd /tmp && git clone --branch=msys2-makefile https://github.com/luau-project/luasocket
  7. Leave the shell opened for the test

Test

  1. In the same shell above, build luasocket for ucrt64 environment:
    make -C luasocket/src PLAT=msys2ucrt64 LUAV=5.1 all
  2. Install the library:
    make -C luasocket/src PLAT=msys2ucrt64 LUAV=5.1 install
  3. Run a simple test
    lua5.1 luasocket/test/hello.lua
  4. Enjoy.

Note

Lua C modules get installed at /ucrt64/lib/lua/5.1 and .lua files at /ucrt64/share/lua/5.1 for the ucrt64 environment, in case you want to remove them.

Extra

If you guys want, I can contribute a Github workflow to test luasocket on MSYS2 tools.

@catwell
Copy link
Member

catwell commented Jul 8, 2024

@luau-project I think better msys2 support is great! Did you test with other Lua versions than 5.1, in particular 5.4?

A GitHub workflow would be great too, since I think most contributors to LuaSocket do not have a Windows machine with msys2 set up. I have not used it in a long time and just noticed they provide an official GitHub Action now.

@luau-project
Copy link
Author

@luau-project I think better msys2 support is great! Did you test with other Lua versions than 5.1, in particular 5.4?

A GitHub workflow would be great too, since I think most contributors to LuaSocket do not have a Windows machine with msys2 set up. I have not used it in a long time and just noticed they provide an official GitHub Action now.

Hello @catwell . I tested it on every 64 bit environment on MSYS2 (but not the ARM one). I even submitted a pull request on MSYS2 msys2/MINGW-packages#21344 to add luasocket on their repositories based on this makefile changes.

As you can see on the MSYS2 PR, it is building fine for every 64 bit env. I didn't test on 32 bit envs though, because I do not have such a machine. However, I'm still confident it does.

@luau-project
Copy link
Author

@luau-project I think better msys2 support is great! Did you test with other Lua versions than 5.1, in particular 5.4?

A GitHub workflow would be great too, since I think most contributors to LuaSocket do not have a Windows machine with msys2 set up. I have not used it in a long time and just noticed they provide an official GitHub Action now.

Just a small notice: on MSYS2, the available versions are current Lua (5.4), Lua 5.3 and Lua 5.1. There is no Lua 5.2.

@luau-project
Copy link
Author

Hello @catwell .

Based on build.yml, I have finished a working msys2.yml GitHub workflow for the makefile changes above.

Should I push this workflow? (I'm supposing you have write access to the repository).

Just in case, I'll write the workflow here. I'm waiting your signal to push it:

name: Build on MSYS2

on:
  push:
    branches:
      - master
  pull_request:

jobs:
  build:
    name: Test ${{ matrix.Lua.version }} from MSYS2 package mingw-w64-${{ matrix.MSYS2.env }}-${{ matrix.Lua.msys2_pkg_name }}
    runs-on: windows-latest

    strategy:
      matrix:
        Lua:
          # For future updates:
          #   the fields 'msys2_pkg_name' and 'msys2_lua_exe'
          #   in the matrix below are always 'lua'
          #   for the current Lua version . 
          - { version: '5.4', msys2_pkg_name: 'lua', msys2_lua_exe: 'lua' }
          - { version: '5.3', msys2_pkg_name: 'lua53', msys2_lua_exe: 'lua5.3' }

          # At the moment, Lua 5.2 is not on MSYS2 repositories.

          - { version: '5.1', msys2_pkg_name: 'lua51', msys2_lua_exe: 'lua5.1' }
          - { version: '5.1', msys2_pkg_name: 'luajit', msys2_lua_exe: 'luajit' }
        
        MSYS2:
          - { sys: ucrt64,  env: ucrt-x86_64 }
          - { sys: mingw64, env: x86_64 }
          - { sys: mingw32, env: i686 }
          - { sys: clang64, env: clang-x86_64 }
          - { sys: clang32, env: clang-i686 }
    
    defaults:
      run:
        shell: msys2 {0}
    
    env:
      LUA_EXE: /${{ matrix.MSYS2.sys }}/bin/${{ matrix.Lua.msys2_lua_exe }}

    steps:
      - uses: msys2/setup-msys2@v2
        name: Setup MSYS2
        with:
          msystem: ${{ matrix.MSYS2.sys }}
          install: |
            base-devel
            git
            mingw-w64-${{ matrix.MSYS2.env }}-cc
            mingw-w64-${{ matrix.MSYS2.env }}-${{ matrix.Lua.msys2_pkg_name }}
      
      - name: Checkout
        uses: actions/checkout@v4
      
      - name: Build
        if: ${{ !contains(matrix.Lua.msys2_pkg_name, 'luajit') }}
        run: |
          make -C src \
            PLAT=msys2${{ matrix.MSYS2.sys }} \
            LUAV=${{ matrix.Lua.version }} \
            DEBUG=DEBUG \
            all

      - name: Build with luajit
        if: ${{ contains(matrix.Lua.msys2_pkg_name, 'luajit') }}
        run: |
          make -C src \
            PLAT=msys2${{ matrix.MSYS2.sys }} \
            LUAV=${{ matrix.Lua.version }} \
            DEBUG=DEBUG \
            "MYCFLAGS=$(pkgconf.exe --cflags lua${{ matrix.Lua.version }})" \
            all

      - name: Install
        run: |
          make -C src \
            PLAT=msys2${{ matrix.MSYS2.sys }} \
            LUAV=${{ matrix.Lua.version }} \
            DEBUG=DEBUG \
            install
        
      - name: Run regression tests
        run: |
          cd test
          ${{ env.LUA_EXE }} hello.lua
          ${{ env.LUA_EXE }} testsrvr.lua > /dev/null &
          ${{ env.LUA_EXE }} testclnt.lua
          ${{ env.LUA_EXE }} stufftest.lua
          ${{ env.LUA_EXE }} excepttest.lua
          ${{ env.LUA_EXE }} test_bind.lua
          ${{ env.LUA_EXE }} test_getaddrinfo.lua
          ${{ env.LUA_EXE }} ltn12test.lua
          ${{ env.LUA_EXE }} mimetest.lua
          ${{ env.LUA_EXE }} urltest.lua
          ${{ env.LUA_EXE }} test_socket_error.lua
          kill %1

@catwell
Copy link
Member

catwell commented Jul 8, 2024

I do have write access to the repository and I don't see a problem with you pushing the workflow. I will probably wait for feedback from @Tieske or @alerque to merge but I can run the workflow in the meantime.

By the way, is your nickname in any way related to @luau-lang?

@luau-project
Copy link
Author

I do have write access to the repository and I don't see a problem with you pushing the workflow. I will probably wait for feedback from @Tieske or @alerque to merge but I can run the workflow in the meantime.

Pushed it.

By the way, is your nickname in any way related to @luau-lang?

Oh, no. When I created this gh acc, I didn't even know the roblox luau.

@catwell
Copy link
Member

catwell commented Jul 12, 2024

Sorry, it took me some time to run the workflows because the new one didn't appear in the list so I was wondering what I had to do... It turns out when you allow the run they still get picked up and run anyway!

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

Other than the nitpick about where to put the CI bits this looks fine to me. I'll me taking for word that the Windows side of things is setup properly because I have no relevant experience or local testing opportunities.

pull_request:

jobs:
build:
Copy link
Member

Choose a reason for hiding this comment

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

Can we add these builds as a new job under the existing Build workflow instead of putting them in a different workflow file? Conceptually what is being tested here seems to be a pretty good match for what is is there. I'm not sure if it makes sens to merge them as part of a single matrix for one job or keep it as two jobs with their own matrices but at least being part of the same workflow makes more sense to me.

Copy link
Author

Choose a reason for hiding this comment

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

done

@alerque
Copy link
Member

alerque commented Jul 15, 2024

Thanks for the contribution. This should become available via the dev (scm) rockspec soon. I don't know when the next release will be, honestly I haven't assessed where this project is at or what it needs before the next tag in a while and I'm kinda busy at the moment. If anybody has input on that ("the SCM version works fine, just send it", "suggest cleaning up X first", "X and Y PRs should land first) I'd be happy to hear input on another issues.

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

Successfully merging this pull request may close these issues.

3 participants