RPGHacker / asar

(Now) official repository of the SNES assembler Asar, originally created by Alcaro
Other
199 stars 42 forks source link

Build asar as a static library too. Add a way to optionally disable building the test app. #243

Closed Atari2 closed 2 years ago

Atari2 commented 2 years ago

With these changes, building asar now also builds a static library version of it. The name of the library will be asar-static.lib/libasar-static.a . This behavior is enabled by default (as everything else) and can be disabled. To do this, add -DASAR_GEN_LIB=OFF to the cmake configuration command. Additionally, this improves the build configuration on MSVC using some cmake generator expressions. It is also now possible to disable the build of the test app with -DASAR_TESTING_DISABLED=TRUE. Note that the build of the test app is enabled by default, to keep the previous behavior.

RPGHacker commented 2 years ago

Wasn't disabling tests already possible via the ASAR_GEN_EXE_TEST and ASAR_GEN_DLL_TEST variables? CMakeLists.txt of asar-tests still checks for those variables.

Atari2 commented 2 years ago

Yeah but I thought providing a single option to just straight up not process the CMakeLists.txt for the test was faster and shorter. If that's too much I can remove that.

RPGHacker commented 2 years ago

It's not really a problem, no. Though I'm not sure if it actually has the desired effect.

Doesn't CMake parse the entire thing once, anyways, so that it knows which options even exist? Or is that only when using CMake GUI?

Atari2 commented 2 years ago

I don't use CMake gui so I have no idea but afaik when invoked from command line CMake just loads the CMakeLists.txt that's present in the folder that was specified, so like cmake src will only load the CMakeLists.txt present in src/ that will then load the rest. So if you prevent the add_subdirectory with an if, I believe that will avoid the whole processing of the new subdirectory entirely.

Atari2 commented 2 years ago

Also it might be worth looking into adding a top-level CMakeLists.txt aswell as having one in src/ because that way other people who use CMake can include asar in their project by using FetchContent like so (cmake will automatically give you the include headers in your project and build the library as needed for linking):

FetchContent_Declare(
    Asar
    GIT_REPOSITORY "https://github.com/RPGHacker/asar"
    GIT_TAG master          # or you can use a github tag with a version number if you wanted fixed source. e.g. 1.81
)

FetchContent_MakeAvailable(Asar)

which doesn't work if the CMakeLists.txt isn't in the top level folder of the github repo. This is just an idea thought, not sure if worth pursuing but it would be super easy to add.

This is a diff that shows how simple the change would be (diff comparing against the asar-static-lib branch)

diff --git a/CMakeLists.txt b/CMakeLists.txt
new file mode 100644
index 0000000..ad4f656
--- /dev/null
+++ b/CMakeLists.txt
@@ -0,0 +1,3 @@
+project(Asar)
+cmake_minimum_required(VERSION 3.9.0)
+add_subdirectory(src)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 8749f23..07b8613 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -1,8 +1,14 @@
-project(Asar)
-
-cmake_minimum_required(VERSION 3.9.0)
-
-add_subdirectory(asar)
-if (NOT ASAR_TESTING_DISABLED)
-       add_subdirectory(asar-tests)
-endif()
\ No newline at end of file
+if(NOT (${PROJECT_NAME} STREQUAL "Asar"))
+       # if the current project name is not Asar
+       # we're being called as top level cmakelists.txt
+       # so it's our job to set the project name.
+       # otherwise, it was already set by our parent cmakelists.txt
+       # so we don't do anything
+       project(Asar)
+endif()
+cmake_minimum_required(VERSION 3.9.0)
+
+add_subdirectory(asar)
+if (NOT ASAR_TESTING_DISABLED)
+       add_subdirectory(asar-tests)
+endif()

I've pushed the diff to here as an example so if you want you can test it out.

This wouldn't be done in this PR but I just thought mentioning it for further thinking is a good idea.

Atari2 commented 2 years ago

Any news on this @RPGHacker ?

RPGHacker commented 2 years ago

In terms of merging this? Nope, not really.

Motiviation and time for hacking-related stuff comes in bursts for me, and unfortunately I'm currently pretty low on both, and also not really in the right headspace. I'm guessing it's a similar story for p4 and randomdude999.

Is it urgent? Do you require this for something specific?

Atari2 commented 2 years ago

Motiviation and time for hacking-related stuff comes in bursts for me, and unfortunately I'm currently pretty low on both, and also not really in the right headspace.

I feel you.

Is it urgent? Do you require this for something specific?

It's not urgent but I require this because I would really like to stop distributing pixi with the asar.dll and instead link it in as a static library, which would help in multiple ways. I could monkey patch it but I really don't want to keep an asar fork up just to have asar as a .lib so having this merged into master would make it really easy. It's fine if you don't want to look at this, I completely understand, I think p4 mentioned to me that he was going to take a look at it sometime this week hopefully.

RPGHacker commented 2 years ago

I see. Yeah, in that case, maybe try bugging p4 or randomdude999. If they don't have the time, I could also grant you collaborator access to the repository so you could add those changes yourself.

There is actually one potential issue that I see with statically linking Asar into PIXI, though. In typical SMW hacking fashion, PIXI currently doesn't seem to specify any license. I'm no legal expert, but as far as I understand, that technically makes it incompatible with open source licenses. Since Asar is licensed under the LGPL, any application statically linking to it would also have to at least use LGPL. Currently, that's not the case with PIXI.

The best thing here would be to retroactively add an LGPL license to PIXI. However, I'm not sure if anyone but JackTheSpades is legally allowed do that - unless maybe we find some old comment/post by them renouncing any rights to PIXI.

Atari2 commented 2 years ago

I see. Yeah, in that case, maybe try bugging p4 or randomdude999. If they don't have the time, I could also grant you collaborator access to the repository so you could add those changes yourself.

There is actually one potential issue that I see with statically linking Asar into PIXI, though. In typical SMW hacking fashion, PIXI currently doesn't seem to specify any license. I'm no legal expert, but as far as I understand, that technically makes it incompatible with open source licenses. Since Asar is licensed under the LGPL, any application statically linking to it would also have to at least use LGPL. Currently, that's not the case with PIXI.

The best thing here would be to retroactively add an LGPL license to PIXI. However, I'm not sure if anyone but JackTheSpades is legally allowed do that - unless maybe we find some old comment/post by them renouncing any rights to PIXI.

Asar ships with 3 licenses, which one is the correct one? Because if I go by https://github.com/RPGHacker/asar/blob/master/license-wtfpl.txt then I'm allowed to do what I want.

Alcaro commented 2 years ago

WTFPL applies to asardll.c and asardll.h, and probably the other language bindings; you're supposed to copy them into your program, can't do that without a permissive license. The actual assembler is LGPL. GPL is included because LGPL is an extra set of permissions on top of GPL, and can't be used alone.

RPGHacker commented 2 years ago

The relevant file is this one, btw. It explains in detail which of the other licenses applies to which situation.

Somewhat confusing, I admit, but that's just licensing in a nutshell.

Atari2 commented 2 years ago

After this commit https://github.com/JackTheSpades/SpriteToolSuperDelux/commit/96265eafeba886c7d9bd92afdc910a5f64bced4d now pixi has a license compatible with asar, so this PR should be looked at again, maybe it's worth it :)

randomdude999 commented 2 years ago

okay but can you please fix the diffs (they show up as having changed every single line in the files you touched, impossible to review)

Atari2 commented 2 years ago

I have no clue what happened to the diffs. I'll just checkout master and redo the changes, they're simple and trivial so it should be no problem. I'll also remove the additional way I added to disable tests since as discussed previously with RPGHacker it's already possible (just a bit more verbose)

Alcaro commented 2 years ago

Looks like someone changed every LF to CRLF, or other way round.

You can get a sane diff at https://github.com/RPGHacker/asar/pull/243/files?w=1, but better fix the patch anyways. It'll screw up git blame and stuff, and we presumably want the linebreaks to keep their current style.