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

build: add version info for libasar #309

Merged
merged 5 commits into from
Mar 11, 2024
Merged

Conversation

orbea
Copy link
Contributor

@orbea orbea commented Mar 1, 2024

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
Copy link
Contributor Author

orbea commented Mar 1, 2024

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

@p4plus2
Copy link
Collaborator

p4plus2 commented Mar 1, 2024

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
Copy link
Contributor Author

orbea commented Mar 1, 2024

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
Copy link
Collaborator

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 orbea force-pushed the lib branch 2 times, most recently from 30eb24c to 28616cd Compare March 2, 2024 15:39
@orbea
Copy link
Contributor Author

orbea commented Mar 2, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

Alcaro commented Mar 4, 2024

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
Copy link
Contributor Author

orbea commented Mar 4, 2024

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
Copy link
Contributor

Alcaro commented Mar 4, 2024

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
Copy link
Contributor Author

orbea commented Mar 4, 2024

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
Copy link
Contributor

Alcaro commented Mar 4, 2024

...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
Copy link
Contributor Author

orbea commented Mar 4, 2024

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

  • Use GNUInstallDirs, when installed on unix systems there is no universally correct install path so its useful to let users set these themselves.
  • Rename asar/interface-lib.h to asar-dll-bindings/c/asar.h.
  • Install the public header to ${CMAKE_INSTALL_INCLUDEDIR}/asar.
  • Generate and install the libasar.pc pkgconfig file, this is useful for downstream projects. The JoinPaths.cmake is from this PR. Unfortunately cmake doesn't have a well defined way to do this correctly...

Name: lib@ASAR_PROJECT_NAME@
URL: https://github.com/RPGHacker/asar
Description: SNES assembler
Version: @ASAR_API_VERSION@
Copy link
Collaborator

Choose a reason for hiding this comment

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

i assume we have to use the API version instead of the real version here? ....maybe we should just delete the apiversion stuff and make the regular version number follow semver instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the library version should match the version in the pkgconfig file, but its really personal preference if the library version should match the package version or not.

For example if the library and package share the version it will require all downstream users to rebuild if the package's major or minor version has been incremented even if the library's API has not changed, but I am not sure that will ever be a real concern with asar?

Copy link
Collaborator

Choose a reason for hiding this comment

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

given our track record regarding backwards compatibility, maybe forcing downstream to do something manually on an asar update wouldn't be such a bad idea. but yes, i did forget this detail of libraries on linux. technically those rebuilds wouldn't be necessary (because on windows, we need to keep ABI compatibility anyways), but i guess there's no way to tell pkg-config about that. so i guess it's fine to keep this as the api version.

@randomdude999
Copy link
Collaborator

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.

@randomdude999 randomdude999 merged commit b885b8d into RPGHacker:asar_19 Mar 11, 2024
3 checks passed
@orbea orbea deleted the lib branch March 11, 2024 15:44
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.

4 participants