martinpitt / umockdev

Mock hardware devices for creating unit tests and bug reporting
https://launchpad.net/umockdev
GNU Lesser General Public License v2.1
312 stars 58 forks source link

Link umockdev-record with libumockdev #148

Closed martinpitt closed 3 years ago

martinpitt commented 3 years ago

Avoid the duplicate static linking of the ioctl sources in umockdev-record, and link it to libumockdev instead. The corresponding symbols are already public in the C API anyway (as they match src/umockdev.map), so just move the previously internal classes/methods to public.

This also avoids confusing complaints about allegedly unused methods.

Fixes #138

martinpitt commented 3 years ago

This can also be seen in https://salsa.debian.org/debian/umockdev/-/blob/master/debian/libumockdev0.symbols , all these supposedly-private symbols are there, like umockdev_ioctl_spi_recorder_construct(). If we want to avoid that, I think we rather need more fine-grained patterns in the map file.

@benzea, WDYT?

benzea commented 3 years ago

Hmm, I didn't really want to make the path registration public and commit to the API there. Maybe not a big deal to have them as symbols though, as long as they are not documented.

martinpitt commented 3 years ago

This drives me nuts.. I tried with the below patch to make this API conditional on compiling the library and umockdev-record:

diff --git meson.build meson.build
index ecafcde..215b511 100644
--- meson.build
+++ meson.build
@@ -122,7 +122,7 @@ umockdev_lib = shared_library('umockdev',
     '-Wl,--no-undefined',
     '-Wl,--version-script,@0@/umockdev.map'.format(srcdir),
   ],
-  vala_args: [ '--vapidir=@0@/src'.format(meson.current_source_dir()) ],
+  vala_args: [ '--define=INTERNAL_REGISTER_API', '--vapidir=@0@/src'.format(meson.current_source_dir()) ],
   include_directories: include_directories('src'),
   version: lib_version,
   install: true,
@@ -173,7 +173,7 @@ umockdev_record_exe = executable('umockdev-record',
    'src/debug.c'],
   dependencies: [glib, gobject, gio_unix, vapi_posix, vapi_config, vapi_ioctl, libpcap],
   link_with: [umockdev_utils_lib],
-  vala_args: [ '--vapidir=@0@/src'.format(meson.current_source_dir()) ],
+  vala_args: [ '--define=INTERNAL_REGISTER_API', '--vapidir=@0@/src'.format(meson.current_source_dir()) ],
   include_directories: include_directories('src'),
   install: true)

diff --git src/umockdev-ioctl.vala src/umockdev-ioctl.vala
index a286faa..2d579e7 100644
--- src/umockdev-ioctl.vala
+++ src/umockdev-ioctl.vala
@@ -741,6 +741,7 @@ public class IoctlBase: GLib.Object {
           listeners.remove(devnode);
     }

+#if INTERNAL_REGISTER_API
     internal void register_path(GLib.MainContext? ctx, string devnode, string sockpath)
     {
         assert(DirUtils.create_with_parents(Path.get_dirname(sockpath), 0755) == 0);
@@ -782,6 +783,7 @@ public class IoctlBase: GLib.Object {
             });
         }
     }
+#endif // INTERNAL_REGISTER_API

     public virtual signal void client_connected(IoctlClient client) {
     }

But this doesn't work: If I leave out the --define for umockdev-record, the API (of course) does not exist:

FAILED: umockdev-record.p/src/umockdev-record.c umockdev-record.p/src/umockdev-ioctl.c umockdev-record.p/src/umockdev-pcap.c umockdev-record.p/src/umockdev-spi.c 
valac -C --debug --debug --pkg libpcap /var/home/martin/upstream/umockdev/src/ioctl.vapi /var/home/martin/upstream/umockdev/src/config.vapi --pkg posix --pkg gio-unix-2.0 --pkg gobject-2.0 --target-glib ' 2.32.0' --pkg glib-2.0 --color=always --directory umockdev-record.p --basedir ../ --vapidir=/var/home/martin/upstream/umockdev/src ../src/ioctl_tree.vapi ../src/umockdev-record.vala ../src/umockdev-ioctl.vala ../src/umockdev-pcap.vala ../src/umockdev-spi.vala umockdev-utils.vapi
../src/umockdev-record.vala:333.5-333.25: error: The name `register_path' does not exist in the context of `UMockdev.IoctlBase?'
    handler.register_path(null, dev, sockpath);
    ^^^^^^^^^^^^^^^^^^^^^
../src/umockdev-record.vala:479.9-479.30: error: The name `unregister_all' does not exist in the context of `UMockdev.IoctlBase?'
        handler.unregister_all();
        ^^^^^^^^^^^^^^^^^^^^^^
Compilation failed: 2 error(s), 0 warning(s)

But if I put in the define, it complains, even though umockdev-record clearly calls the API:

[1/6] valac -C --debug --debug --pkg libpcap /var/home/martin/upstream/umockdev/src/ioctl.vapi /var/home/martin/upstream/umockdev/src/config.vapi --pkg posix --pkg gio-unix-2.0 --pkg gobject-2.0 --target-glib ' 2.32.0' --pkg glib-2.0 --color=always --directory umockdev-record.p --basedir ../ --define=INTERNAL_REGISTER_API --vapidir=/var/home/martin/upstream/umockdev/src ../src/ioctl_tree.vapi ../src/umockdev-record.vala ../src/umockdev-ioctl.vala ../src/umockdev-pcap.vala ../src/umockdev-spi.vala umockdev-utils.vapi
../src/umockdev-ioctl.vala:772.5-772.33: warning: method `UMockdev.IoctlBase.unregister_path' never used
    internal void unregister_path(string devnode)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Compilation succeeded - 1 warning(s)
martinpitt commented 3 years ago

Oh, stupid me -- of course it really is not being used; umockdev-record only uses unregister_all(). I'll send a new PR, I have this working now.