5:32 p.m.
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(a)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)
5:45 p.m.
On Sat, Aug 22, 2020 at 6:34 PM Scott Shambarger
<scott-libvirt(a)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(a)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!
7:03 p.m.
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
4:34 a.m.
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 :|
8:15 p.m.
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
3:44 a.m.
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 :|
4:29 a.m.
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
4:36 a.m.
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 :|
6:47 p.m.
New subject: [PATCH v2] util: use host module suffix when loading drivers
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(a)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)
3:34 a.m.
New subject: [PATCH v2] util: use host module suffix when loading drivers
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(a)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(a)redhat.com>
And pushed.
1549
days inactive
1553
days old
10 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
Neal Gompa -
Pavel Hrdina -
Scott Shambarger