meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.25k stars 2.48k forks source link

[1.x] Mac: Fails to launch after .dylib versioning scheme change #3212

Closed aet closed 1 year ago

aet commented 1 year ago

There's a small issue with JANUS_VERSION_SO / libtool version-info handling on a Mac, that I'm occasionally using for development even if the actual stuff gets deployed with Linux.

src/janus.c tries to avoid loading multiple copies of installed .dylib files by preferring .0.dylib ones.

Once https://github.com/meetecho/janus-gateway/commit/c940c306c1f9de59754575c09a47315a3fe53af3 was committed, the default .dylib versionining scheme changed from .0.dylib to .1.dylib. If you try running against a clean environment without existing plugins, Janus fails to launch due to no transports found.

What version of Janus is this happening on?

Current master (013155d6e806277f8eee6272388dc95ab5294ef8)

Have you tested a more recent version of Janus too?

Was this working before?

I've been using a v1.0.0 era branch, it seems https://github.com/meetecho/janus-gateway/pull/3075 landed for 1.1.0 that set the baseline for version handling from default 0:0:0, followed by https://github.com/meetecho/janus-gateway/commit/c940c306c1f9de59754575c09a47315a3fe53af3

Is there a gdb or libasan trace of the issue?

Additional context

lminiero commented 1 year ago

I hadn't taken MacOS into account when we made the change, mostly because I don't own a MacOS machine and so never test builds there (and neither do we include MacOS in our CI builds, for that matter). If you have suggestions on how to address this, that would help, as I don't know the naming conventions there.

lminiero commented 1 year ago

@aet does changing line 53 in janus.c from this

#define SHLIB_EXT "0.dylib"

to this

#define SHLIB_EXT ".dylib"

help? It's what we do for shared objects on Linux (#define SHLIB_EXT ".so"), so I'd expect the same to be ok on MacOS as well (unless we generate .dylib filenames differently in the Makefile -- I'll check).

aet commented 1 year ago

That was my first attempt, but it's resulting to many instances of the plugin being loaded. :(

The main difference probably is that .so's typically append version info after .so, and .dylib's maintain the major version part before .dylib. I started some time ago to add a simple shlib_filter helper function to avoid repeating the readdir iterations specifics, if it gets more complicated, but it might take a week or few to get back to finalizing it. Happy to do a PR though, it's not a big urgency for me personally

Alternatively should the plugins have some extra validation if X is already loaded through a different filename? That way just sticking with ".dylib" should work.

lminiero commented 1 year ago

That was my first attempt, but it's resulting to many instances of the plugin being loaded. :(

The main difference probably is that .so's typically append version info after .so, and .dylib's maintain the major version part before .dylib.

Ah I wasn't aware of the difference. Yeah in Linux the version after the .so does fix it, because the only .so in the folder is the symlink to the latest version. Maybe we can just change the 0.dylib to 1.dylib then?

Alternatively should the plugins have some extra validation if X is already loaded through a different filename? That way just sticking with ".dylib" should work.

I'm not sure that would work, because you'd have no guarantee that the latest version is the one loaded first.

aet commented 1 year ago

That's also possible, but then as a 3rdparty plugin developer you'll need to follow JANUS_VERSION_SO versioning and ensure -version-info is used in your build process properly :) There's probably no tooling to query that information after Janus is installed?

Would be easier to just pick up .dylib after some extra validation the filename doesn't match .ver., so it's doing the same as .so where .so likely symlinks to the latest version. Changing to 1.dylib fixes the out of the box experience for now, I might try giving an extra validation pass against .dylib later!

lminiero commented 1 year ago

Ack, I'll push the 1.dylib fix for the time being then, and I'll wait for a PR in case you come up with a more proper fix.

lminiero commented 1 year ago

I'll close this issue as I assume the 1.dylib fixed the problem for the time being, but of course I'll welcome a PR that fixes it more properly in the future. Thanks for making me aware of the problem!