Add precompiled StormLib for Linux #84
Conversation
There was a problem hiding this comment.
Hi @vec2pt, first of all thank you for your contribution!
May I ask your interest or how you may plan to use this library? I haven't actually publicized it anywhere yet as I'm trying to finish out all the Trigger actions and improve examples/documentation before letting others know.
RichChk does currently provide copies of StormLib for the most common operating systems, but long term I don't think this is sustainable, as OS/architectures will inevitably change and the DLLs will need to be re-compiled for future platforms. As the warning shows in stormlib_finder.py, this behavior is discouraged and ideally users provide their own DLL not relying on RichChk to supply this.
Second, there's a few set of unit tests which only run on macOS. All the tests related to MPQ/StormLib functions found in https://github.com/sethmachine/richchk/tree/1a8792ce5e457aae21adcf4e5d30c4f5d9f7fd8b/test/mpq. To prove that this change is safe, could you enable tests for Linux and see if all the unit tests pass? And the additional unit tests that run on ubuntu viahttps://github.com/sethmachine/richchk/blob/72a35843142d810a5de894178b8981ca7a2aa5d8/.github/workflows/pre-commit.yaml#L23-L28
Lastly, looks like the pre-commit step for macOS is failing with this PR. I think that needs to be fixed too.
| @@ -18,6 +18,7 @@ class StormLibFinder: | |||
| _MAC_STORM_INTEL = os.path.join(_SCRIPT_PATH, "dlls/macos/libStorm.dylib") | |||
| _MAC_STORM_M1 = os.path.join(_SCRIPT_PATH, "dlls/macos/libstorm.9.22.0.dylib") | |||
| _WINDOWS_STORM = os.path.join(_SCRIPT_PATH, "dlls/windows/Storm.dll") | |||
| _LINUX_STORM = os.path.join(_SCRIPT_PATH, "dlls/linux/libstorm.so.9.22.0") | |||
There was a problem hiding this comment.
Are there other CPU architectures (e.g. 32 vs 64 bit) for Linux distributions? If so, maybe rename this to _LINUX_STORM_X86_64.
Or is it really unlikely to support a 32-bit distribution?
There was a problem hiding this comment.
Sure I will change the name.
| @@ -29,7 +30,7 @@ def find_stormlib( | |||
| else: | |||
| cls._LOG.warning( | |||
| "No path to a StormLib DLL was provided, " | |||
| "attempting to use precompiled DLL for Windows and macOS. " | |||
| "attempting to use precompiled DLL for Windows, macOS and Linux. " | |||
There was a problem hiding this comment.
Will this work for every flavor of 64-bit Linux? Ubuntu, centos, Debian, etc.?
How was this DLL compiled? Did you compile it? Or where did it come from?
There was a problem hiding this comment.
Yes, it should work.
The library was compiled by myself. I am using Debian 12.
There was a problem hiding this comment.
Thanks for letting me know! Only thing I would add, could you provide the instructions you did to compile the StormLib DLL on your system? Either in the PR request or a link to something, etc. Wanting to make sure it can be repeated. E.g. for macOS StormLib, Homebrew builds it: https://formulae.brew.sh/formula/stormlib
There was a problem hiding this comment.
The instructions are described in the StormLib repository: https://github.com/ladislav-zezula/StormLib?tab=readme-ov-file#linux. One thing I added the BUILD_SHARED_LIBS=True flag to cmake.
git clone https://github.com/ladislav-zezula/StormLib.git
mkdir StormLib/build && cd StormLib/build
cmake ../CMakeLists.txt -D BUILD_SHARED_LIBS=True
makeand in the build folder will be compiled library libstorm.so.9.22.0
| mock_platform_system.return_value = "Linux" | ||
| mock_cpu.return_value = "i386" |
There was a problem hiding this comment.
this would technically be an unrecognized CPU instruction set? You could put something made-up here like iOS or Opera (or any value would work). Since now you're adding support for Linux, I would take it out of this test as the trigger for the error.
|
Hi @sethmachine . I started to write a simple script to organize my collection of maps for Starcraft and extract the basic information (such as map size, number of players, etc.), in addition I would like to play around with the analysis of these maps. I think this is a good idea. In any case, I think that temporarily it would be nice to have Linux support from the box. Sure :) The nearest time will write unit tests. I don't see the pre-commit step fails. Could you provide me with the path to the logs? |
|
Looks like something may have broke the pre-commit (unrelated to your change): https://stackoverflow.com/questions/79057817/invalidmanifesterror-at-key-language-expected-one-of-but-got-python-venv I'm going to try to fix this so your PR can be merged in the next few days. |
|
It's a known issue that hopefully is resolved soon: PyCQA/docformatter#289 I have a fix in place that should unblock this by keeping pre-commit at version 3.5.0: #92 |
|
@vec2pt if you merge master in your branch the pre-commit hooks can be re-run and hopefully pass. Then I can mark PR for approval and merge in. I fixed the pre-commit issue and merged in master. |
Thanks for letting me know! You can see the list of supported CHK sections here: https://github.com/sethmachine/richchk/tree/master/src/richchk/model/richchk. I focused on modeling all the sections required to read and write Trigger data / unit settings. So if you want something for player settings, like OWNR, you would have to add this to the library: http://www.staredit.net/wiki/index.php/Scenario.chk#.22OWNR.22_-_StarCraft_Player_Types. I can eventually make a guide on how to do that, but you can look at the existing code to see the pattern for doing so. |
Thanks for the info :) |
Closes #83