RPGHacker / asar

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

build: add version info for libasar #309

Closed orbea closed 8 months ago

orbea commented 8 months ago

This adds version info for libasar.so which now installs these files.

/usr/lib/libasar.so -> libasar.so.0
/usr/lib/libasar.so.0 -> libasar.so.0.0.0
/usr/lib/libasar.so.0.0.0

This is useful to mark API changes in libasar.so and is more consistent with other libraries installed on the system. This version probably should be independent of the main asar 1.90 version so it is only bumped with the library itself changes. If another version than 0.0.0 is desired I can change that, but I suggest following semantic versioning for the library.

reference: https://semver.org/

orbea commented 8 months ago

Seems the windows appveyor builds are not happy about this, but I am not really sure what the expected behavior is there?

p4plus2 commented 8 months ago

We do also have #define expectedapiversion 303 in the library header, but I'm not opposed to this idea. I'm currently traveling and can't dig into the appveyor issue and will have to defer to trillian or rpghacker for now.

orbea commented 8 months ago

To be clear my goal is to make asar more friendly for Linux distros so that it would be possible to provide distro packages and in my specific case I am testing against Gentoo. At least for unix systems having the versioned libraries would be beneficial in addition to the already existing define in the header.

I'm currently traveling and can't dig into the appveyor issue and will have to defer to trillian or rpghacker for now.

No worries, whenever you or someone else has time to help it would be greatly appreciated.

randomdude999 commented 8 months ago

i'm guessing it doesn't like you declaring a 2nd project. do you need the project here? can't this just be a variable?

as for the value, the current expectedapiversion system is used to verify compatibility at runtime. it corresponds roughly to semver "major" and "minor" components. so i guess we can set this version to 3.3.0 and auto-generate expectedapiversion from it (as major*100+minor) (i don't expect us to ever need the 3rd component for just the API).

also: setting the cmake VERSION property here will make cmake use that as the image version on windows. currently we have some .rc files that set the image version to the real asar version number. should check which one gets precedence here

orbea commented 8 months ago

i'm guessing it doesn't like you declaring a 2nd project. do you need the project here? can't this just be a variable?

Yes, that is better, I am relatively inexperienced with cmake.

as for the value, the current expectedapiversion system is used to verify compatibility at runtime. it corresponds roughly to semver "major" and "minor" components. so i guess we can set this version to 3.3.0 and auto-generate expectedapiversion from it (as major*100+minor) (i don't expect us to ever need the 3rd component for just the API).

I changed it to 3.3.0.

also: setting the cmake VERSION property here will make cmake use that as the image version on windows. currently we have some .rc files that set the image version to the real asar version number. should check which one gets precedence here

Unfortunately my windows knowledge is lacking and I don't have access to any windows machines.

randomdude999 commented 8 months ago

Unfortunately my windows knowledge is lacking and I don't have access to any windows machines.

running windres on the appveyor artifacts confirms it's fine.

tiny nitpick: as is, the variable probably shouldn't be called ASAR_VERSION, because that's not what it is. should be called ASAR_API_VERSION or something. but i can fix that myself. i've wanted to move the real version number into CMakeLists for a while anyways, because it's in more than 1 place currently and people keep forgetting to update all the places.

Alcaro commented 8 months ago

libasar.so is typically loaded with dlopen, not ld-level linking; our header file only supports dlopen, and our recommended dlopen caller hardcodes a filename not containing a soversion.

So I'm not sure if anything would use said soversion.

...maybe it should, that filename list is easy to edit. Though getting the api version into a string may be slightly less easy.

orbea commented 8 months ago

libasar.so is typically loaded with dlopen, not ld-level linking

Thanks for pointing that out, when I was examining the global symbols with nm(1) I made the wrong assumption that it could be linked at the ld-level. If its strictly dlopen maybe this doesn't make as much sense as I thought, but maybe there would be value with being able to link the library too?

Although for a loadable module it might be better to name it asar.so rather than libasar.so as documented for automake modules.

https://www.gnu.org/software/automake/manual/html_node/Libtool-Modules.html

Alcaro commented 8 months ago

There's nothing dlopen-specific about libasar.so, it'd work perfectly fine if the C header was adjusted. The main reason for this design is Windows linkers need some funny import lib file, not just the DLL, and I didn't feel like figuring out how to create that.

(Whether adjusting our headers is worth the effort is, of course, a completely different question.)

Adding soversion can help dlopen if it's given a versioned filename, just like ld-level linking; if we merge this, we should add the versioned name to the bindings files. (We can keep the unversioned name as a fallback.)

Naming it libasar.so is partially because I was clueless when I wrote that thing, partially because some FFI libraries are stupid and hardcode a 'lib' prefix even if you don't ask for one.

orbea commented 8 months ago

Even when its versioned at least on ELF and MACHO systems it will still create the libasar.(so/dylib) symlink so that anything that expected that filename could still find it.

However if libasar.so is linked then there would also need to be some kind of public header that can be included in other projects, I think interface-lib.h is basically that already? I'm not sure it would need any changes other than being installed?

Alcaro commented 8 months ago

...huh, I didn't know that exists.

Yes, that certainly would be suitable as a header for a ld-linked libasar.so (judging by commit message, it's intended for linking Asar statically, but it works for this too). If it already exists, then there's no effort needed to adjust our headers, and that argument is safe to ignore.

Not a huge fan of installing anything with that filename anywhere, though. Better rename to asar.h or libasar.h or something. Maybe also move it to the dll bindings directory, for increased discoverability.

orbea commented 8 months ago

Agreed about the filename, I rebased and added several more commits which I think completes the ideas in this PR.

randomdude999 commented 8 months ago

Adding soversion can help dlopen if it's given a versioned filename, just like ld-level linking; if we merge this, we should add the versioned name to the bindings files. (We can keep the unversioned name as a fallback.)

i'm not sure how this would interact with the "replace dll with a newer version of asar" use case. could cause apps to load an older asar when a newer one is present... though if you have 2 different version-suffixed asars present, you probably know what you're doing anyways.