[PATCH] src/meson: add module name_suffix as 'so'
driver.c and locking/lock_manager.c both hardcode ".so" to module filenames. In meson, MacOS bundles (loadable objects) default to using ".dylib" - adding name_suffix as "so" will therefore align the code with all builds (no need to handle windows as it doesn't support libvirtd). Signed-off-by: Scott Shambarger <scott-libvirt@shambarger.net> --- src/meson.build | 1 + 1 file changed, 1 insertion(+) diff --git a/src/meson.build b/src/meson.build index 73ac99f01e..73986401c9 100644 --- a/src/meson.build +++ b/src/meson.build @@ -567,6 +567,7 @@ foreach module : virt_modules module['name'], module.get('sources', []), name_prefix: module.get('name_prefix', 'lib'), + name_suffix: 'so', include_directories: [ conf_inc_dir, module.get('include', []), -- 2.24.3 (Apple Git-128)
On Sat, Aug 22, 2020 at 6:34 PM Scott Shambarger <scott-libvirt@shambarger.net> wrote:
driver.c and locking/lock_manager.c both hardcode ".so" to module filenames. In meson, MacOS bundles (loadable objects) default to using ".dylib" - adding name_suffix as "so" will therefore align the code with all builds (no need to handle windows as it doesn't support libvirtd).
Signed-off-by: Scott Shambarger <scott-libvirt@shambarger.net> --- src/meson.build | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/meson.build b/src/meson.build index 73ac99f01e..73986401c9 100644 --- a/src/meson.build +++ b/src/meson.build @@ -567,6 +567,7 @@ foreach module : virt_modules module['name'], module.get('sources', []), name_prefix: module.get('name_prefix', 'lib'), + name_suffix: 'so', include_directories: [ conf_inc_dir, module.get('include', []), -- 2.24.3 (Apple Git-128)
Shouldn't it be fixed the other way? That is, it should know about DLL, SO, or DYLIB extensions depending on the platform... Also, libvirtd would probably work on Cygwin or Midipix, which would use DLL files... -- 真実はいつも一つ！/ Always, there's only one truth!
On 2020-08-22 15:45, Neal Gompa wrote:
Shouldn't it be fixed the other way? That is, it should know about DLL, SO, or DYLIB extensions depending on the platform...
Well, there's a myriad of discussions regarding this on projects from glib to hexchat (and I read quite a few of them :)... basically, libtool has used .so for modules ("bundles" on MacOS) for 14 years, as does MacPorts and Homebrew -- it might just be easier to follow the crowd...
Also, libvirtd would probably work on Cygwin or Midipix, which would use DLL files...
There's more complexity there... since windows doesn't add the "lib" prefix, driver.c would need to handle that edge-case as well... libvirtd has historically used ".so" on MacOS (via libtool). Any "externally" installed drivers would break if that changed (are there any?). I'd be game to work on a patch that uses the whatever meson does, but I don't think it currently has a specific config for the suffix used for loadable modules (which, on MacOS should really be ".bundle", but is currently ".dylib" - just to be even more confused :). Thoughts? Scott
On Sat, Aug 22, 2020 at 05:03:45PM -0700, Scott Shambarger wrote:
On 2020-08-22 15:45, Neal Gompa wrote:
Shouldn't it be fixed the other way? That is, it should know about DLL, SO, or DYLIB extensions depending on the platform...
Well, there's a myriad of discussions regarding this on projects from glib to hexchat (and I read quite a few of them :)... basically, libtool has used .so for modules ("bundles" on MacOS) for 14 years, as does MacPorts and Homebrew -- it might just be easier to follow the crowd...
I don't think we want to keep compatibility with any libtool quirks. We should do the right thing for the platform, and I think that means using the platform native suffix for shared library modles correctly.
Also, libvirtd would probably work on Cygwin or Midipix, which would use DLL files...
We don't support Cygwin, only Mingw
There's more complexity there... since windows doesn't add the "lib" prefix, driver.c would need to handle that edge-case as well...
libvirtd has historically used ".so" on MacOS (via libtool). Any "externally" installed drivers would break if that changed (are there any?).
We don't support any use of 3rd party modules. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2020-08-24 02:34, Daniel P. Berrangé wrote:
I don't think we want to keep compatibility with any libtool quirks. We should do the right thing for the platform, and I think that means using the platform native suffix for shared library modles correctly.
Of course... we'd still have to deal with meson quirks (it suffixes modules with .dylib instead of .bundle on MacOS). I'm happy to take a crack at a patch using the platform suffix in the code... any idea how to configure meson to put the module suffix into config.h? (I'm kinda new to meson - would be easy in autoconf :) Thanks, Scott
On Mon, Aug 24, 2020 at 06:15:57PM -0700, Scott Shambarger wrote:
On 2020-08-24 02:34, Daniel P. Berrangé wrote:
I don't think we want to keep compatibility with any libtool quirks. We should do the right thing for the platform, and I think that means using the platform native suffix for shared library modles correctly.
Of course... we'd still have to deal with meson quirks (it suffixes modules with .dylib instead of .bundle on MacOS).
I'm happy to take a crack at a patch using the platform suffix in the code... any idea how to configure meson to put the module suffix into config.h? (I'm kinda new to meson - would be easy in autoconf :)
I wouldn't bother - just #ifdef __APPLE__ in the source code Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Aug 25, 2020 at 09:44:41AM +0100, Daniel P. Berrangé wrote:
On Mon, Aug 24, 2020 at 06:15:57PM -0700, Scott Shambarger wrote:
On 2020-08-24 02:34, Daniel P. Berrangé wrote:
I don't think we want to keep compatibility with any libtool quirks. We should do the right thing for the platform, and I think that means using the platform native suffix for shared library modles correctly.
Of course... we'd still have to deal with meson quirks (it suffixes modules with .dylib instead of .bundle on MacOS).
I'm happy to take a crack at a patch using the platform suffix in the code... any idea how to configure meson to put the module suffix into config.h? (I'm kinda new to meson - would be easy in autoconf :)
I wouldn't bother - just #ifdef __APPLE__ in the source code
Meson doesn't export any function to get the default suffix but they have clear definition of the default suffixes: 'By default, for shared libraries this is dylib on macOS, dll on Windows, and so everywhere else.' As we don't care about Windows in this case we can hard-code .dylib in meson or in source code. With meson you would do something like this in meson.build: if host_machine.system() == 'darwin' conf.set('MODULE_SUFFIX', '.dylib') else conf.set('MODULE_SUFFIX', '.so') endif The conf variable is then used to generate meson-config.h which is included in config.h Or as Dan suggested you can do #ifdef somewhere in source code. Pavel
On Tue, Aug 25, 2020 at 11:29:21AM +0200, Pavel Hrdina wrote:
On Tue, Aug 25, 2020 at 09:44:41AM +0100, Daniel P. Berrangé wrote:
On Mon, Aug 24, 2020 at 06:15:57PM -0700, Scott Shambarger wrote:
On 2020-08-24 02:34, Daniel P. Berrangé wrote:
I don't think we want to keep compatibility with any libtool quirks. We should do the right thing for the platform, and I think that means using the platform native suffix for shared library modles correctly.
Of course... we'd still have to deal with meson quirks (it suffixes modules with .dylib instead of .bundle on MacOS).
I'm happy to take a crack at a patch using the platform suffix in the code... any idea how to configure meson to put the module suffix into config.h? (I'm kinda new to meson - would be easy in autoconf :)
I wouldn't bother - just #ifdef __APPLE__ in the source code
Meson doesn't export any function to get the default suffix but they have clear definition of the default suffixes:
'By default, for shared libraries this is dylib on macOS, dll on Windows, and so everywhere else.'
As we don't care about Windows in this case we can hard-code .dylib in meson or in source code. With meson you would do something like this in meson.build:
if host_machine.system() == 'darwin' conf.set('MODULE_SUFFIX', '.dylib') else conf.set('MODULE_SUFFIX', '.so') endif
The conf variable is then used to generate meson-config.h which is included in config.h
I don't think that adds any value over just using an #ifdef in the source. Arguably it makes it worse, as you can't see what's happening in the source without serching for the definition of MODULE_SUFFIX in meson. Autoconf skewed our POV towards using defining config.h variables for everything, because when you have a big hammer, everything looks like a nail. We should resist the urge to just blindly follow the same approach we did with autoconf. Use meson generated config.h variables only where truly adding value. ie things that are a response to user config params to meson, or things that we needed to auto-detect from host OS. ...
Or as Dan suggested you can do #ifdef somewhere in source code.
...If it can just be a siple #ifdef in the source though, just do that and skip meson-confg.h Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Driver module loaders current hardcode ".so" as the file extension. On MacOS, meson uses ".dylib" as a module file extension. This patch adds VIR_FILE_MODULE_EXT to virfile.h defined as the hosts module extension, and updates driver module loaders to make use of it. Signed-off-by: Scott Shambarger <scott-libvirt@shambarger.net> --- src/driver.c | 2 +- src/locking/lock_manager.c | 2 +- src/storage/storage_backend.c | 2 +- src/util/virfile.h | 6 ++++++ src/util/virstoragefilebackend.c | 2 +- 5 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/driver.c b/src/driver.c index 37bf9de392..1cacec24ff 100644 --- a/src/driver.c +++ b/src/driver.c @@ -55,7 +55,7 @@ virDriverLoadModule(const char *name, if (!(modfile = virFileFindResourceFull(name, "libvirt_driver_", - ".so", + VIR_FILE_MODULE_EXT, abs_top_builddir "/src", DEFAULT_DRIVER_DIR, "LIBVIRT_DRIVER_DIR"))) diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index 39e482f11c..ada950e03e 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -138,7 +138,7 @@ virLockManagerPluginPtr virLockManagerPluginNew(const char *name, } else { if (!(modfile = virFileFindResourceFull(name, NULL, - ".so", + VIR_FILE_MODULE_EXT, abs_top_builddir "/src", LIBDIR "/libvirt/lock-driver", "LIBVIRT_LOCK_MANAGER_PLUGIN_DIR"))) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 1f83236f2f..2bce445575 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -91,7 +91,7 @@ virStorageDriverLoadBackendModule(const char *name, if (!(modfile = virFileFindResourceFull(name, "libvirt_storage_backend_", - ".so", + VIR_FILE_MODULE_EXT, abs_top_builddir "/src", STORAGE_BACKEND_MODULE_DIR, "LIBVIRT_STORAGE_BACKEND_DIR"))) diff --git a/src/util/virfile.h b/src/util/virfile.h index 2eec89598f..09488398c5 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -36,6 +36,12 @@ typedef enum { VIR_FILE_CLOSE_DONT_LOG = 1 << 2, } virFileCloseFlags; +#ifdef __APPLE__ +# define VIR_FILE_MODULE_EXT ".dylib" +#else +# define VIR_FILE_MODULE_EXT ".so" +#endif + ssize_t saferead(int fd, void *buf, size_t count) G_GNUC_WARN_UNUSED_RESULT; ssize_t safewrite(int fd, const void *buf, size_t count) G_GNUC_WARN_UNUSED_RESULT; diff --git a/src/util/virstoragefilebackend.c b/src/util/virstoragefilebackend.c index bf452a1dc5..2779b5c307 100644 --- a/src/util/virstoragefilebackend.c +++ b/src/util/virstoragefilebackend.c @@ -56,7 +56,7 @@ virStorageFileLoadBackendModule(const char *name, if (!(modfile = virFileFindResourceFull(name, "libvirt_storage_file_", - ".so", + VIR_FILE_MODULE_EXT, abs_top_builddir "/src", STORAGE_FILE_MODULE_DIR, "LIBVIRT_STORAGE_FILE_DIR"))) -- 2.24.3 (Apple Git-128)
On Tue, Aug 25, 2020 at 04:47:07PM -0700, Scott Shambarger wrote:
Driver module loaders current hardcode ".so" as the file extension. On MacOS, meson uses ".dylib" as a module file extension. This patch adds VIR_FILE_MODULE_EXT to virfile.h defined as the hosts module extension, and updates driver module loaders to make use of it.
Signed-off-by: Scott Shambarger <scott-libvirt@shambarger.net> --- src/driver.c | 2 +- src/locking/lock_manager.c | 2 +- src/storage/storage_backend.c | 2 +- src/util/virfile.h | 6 ++++++ src/util/virstoragefilebackend.c | 2 +- 5 files changed, 10 insertions(+), 4 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com> And pushed.
participants (4)
-
Daniel P. Berrangé -
Neal Gompa -
Pavel Hrdina -
Scott Shambarger