diff mbox series

meson: Propagate gnutls dependency

Message ID 20210102125213.41279-1-r.bolshakov@yadro.com (mailing list archive)
State New, archived
Headers show
Series meson: Propagate gnutls dependency | expand

Commit Message

Roman Bolshakov Jan. 2, 2021, 12:52 p.m. UTC
crypto/tlscreds.h includes GnuTLS headers if CONFIG_GNUTLS is set, but
GNUTLS_CFLAGS, that describe include path, are not propagated
transitively to all users of crypto and build fails if GnuTLS headers
reside in non-standard directory (which is a case for homebrew on Apple
Silicon).

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 block/meson.build          | 2 +-
 io/meson.build             | 2 +-
 meson.build                | 5 +++--
 storage-daemon/meson.build | 2 +-
 tests/meson.build          | 6 +++---
 ui/meson.build             | 2 +-
 6 files changed, 10 insertions(+), 9 deletions(-)

Comments

Peter Maydell Jan. 2, 2021, 1:25 p.m. UTC | #1
On Sat, 2 Jan 2021 at 12:54, Roman Bolshakov <r.bolshakov@yadro.com> wrote:
>
> crypto/tlscreds.h includes GnuTLS headers if CONFIG_GNUTLS is set, but
> GNUTLS_CFLAGS, that describe include path, are not propagated
> transitively to all users of crypto and build fails if GnuTLS headers
> reside in non-standard directory (which is a case for homebrew on Apple
> Silicon).
>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>

Ah, this is https://bugs.launchpad.net/qemu/+bug/1909256
-- thanks for finding a fix.

> ---
>  block/meson.build          | 2 +-
>  io/meson.build             | 2 +-
>  meson.build                | 5 +++--
>  storage-daemon/meson.build | 2 +-
>  tests/meson.build          | 6 +++---
>  ui/meson.build             | 2 +-
>  6 files changed, 10 insertions(+), 9 deletions(-)

> diff --git a/ui/meson.build b/ui/meson.build
> index 013258a01c..e6655c94a6 100644
> --- a/ui/meson.build
> +++ b/ui/meson.build
> @@ -29,7 +29,7 @@ vnc_ss.add(files(
>    'vnc-ws.c',
>    'vnc-jobs.c',
>  ))
> -vnc_ss.add(zlib, png, jpeg)
> +vnc_ss.add(zlib, png, jpeg, gnutls)
>  vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
>  softmmu_ss.add_all(when: vnc, if_true: vnc_ss)
>  softmmu_ss.add(when: vnc, if_false: files('vnc-stubs.c'))

Question to Paolo -- it seems pretty fragile to have to explicitly
list "these source files need these extra CFLAGS" in half a dozen
meson.build files, because it's pretty non-obvious that adding
eg '#include "block/nbd.h"' to a .c file means that you also
need to update the meson.build file to say "and now it needs these
extra CFLAGS". Isn't there some way we can just have the CFLAGS
added more globally so that if we use gnutls.h directly or
indirectly from more .c files in future it Just Works ?

If the build failed for the common Linux case then it would be
at least more obvious that you needed to update the meson.build
files. I think it's better to avoid "you need to do this special
thing that you'll only notice you're missing if you happen to test
on a somewhat obscure host configuration" where we can.

(We don't want to link helper binaries etc against gnutls if
they don't need it, but that's LDFLAGS, not CFLAGS.)

thanks
-- PMM
Roman Bolshakov Jan. 2, 2021, 2:16 p.m. UTC | #2
On Sat, Jan 02, 2021 at 01:25:07PM +0000, Peter Maydell wrote:
> On Sat, 2 Jan 2021 at 12:54, Roman Bolshakov <r.bolshakov@yadro.com> wrote:
> >
> > crypto/tlscreds.h includes GnuTLS headers if CONFIG_GNUTLS is set, but
> > GNUTLS_CFLAGS, that describe include path, are not propagated
> > transitively to all users of crypto and build fails if GnuTLS headers
> > reside in non-standard directory (which is a case for homebrew on Apple
> > Silicon).
> >
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> 
> Ah, this is https://bugs.launchpad.net/qemu/+bug/1909256
> -- thanks for finding a fix.
> 

No problem :)

> > ---
> >  block/meson.build          | 2 +-
> >  io/meson.build             | 2 +-
> >  meson.build                | 5 +++--
> >  storage-daemon/meson.build | 2 +-
> >  tests/meson.build          | 6 +++---
> >  ui/meson.build             | 2 +-
> >  6 files changed, 10 insertions(+), 9 deletions(-)
> 
> > diff --git a/ui/meson.build b/ui/meson.build
> > index 013258a01c..e6655c94a6 100644
> > --- a/ui/meson.build
> > +++ b/ui/meson.build
> > @@ -29,7 +29,7 @@ vnc_ss.add(files(
> >    'vnc-ws.c',
> >    'vnc-jobs.c',
> >  ))
> > -vnc_ss.add(zlib, png, jpeg)
> > +vnc_ss.add(zlib, png, jpeg, gnutls)
> >  vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
> >  softmmu_ss.add_all(when: vnc, if_true: vnc_ss)
> >  softmmu_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
> 
> Question to Paolo -- it seems pretty fragile to have to explicitly
> list "these source files need these extra CFLAGS" in half a dozen
> meson.build files, because it's pretty non-obvious that adding
> eg '#include "block/nbd.h"' to a .c file means that you also
> need to update the meson.build file to say "and now it needs these
> extra CFLAGS". Isn't there some way we can just have the CFLAGS
> added more globally so that if we use gnutls.h directly or
> indirectly from more .c files in future it Just Works ?
> 

Right. I converted a big C++ project to CMake 3 a few years ago and was
able to solve the problem in CMake because it properly supports
transitive dependencies.

In CMake I'd specify that crypto has public dependency on gnutls only
once and then all users of crypto (direct or indirect) would get
required CFLAGS, LDFLAGS and include directories.

I spent a few hours trying to figure out how to achieve the same in
meson (without code duplication and failed miserably). Here's a meson
project test that illustrates the problem of dependency duplication:

https://github.com/mesonbuild/meson/commit/ff5dc65ef841857dd306694dff1fb1cd2bf801e4

The project doesn't build because dependency on foo is not propagated
beyond foobar.

The only way to build it is to specify foo twice - in source set of
foobar and in declared_dependency (i.e. appending "dependencies: [foo]"
to declare_dependency helps).

Unfortunately, the approach doesn't work for meson/qemu because it
introduces duplicate symbols in different static libraries. That's why I
used much more uglier "specify headers where needed all over the code
base".

I'd be happy to hear what's the proper way to fix it.

Thanks,
Roman

> If the build failed for the common Linux case then it would be
> at least more obvious that you needed to update the meson.build
> files. I think it's better to avoid "you need to do this special
> thing that you'll only notice you're missing if you happen to test
> on a somewhat obscure host configuration" where we can.
> 
> (We don't want to link helper binaries etc against gnutls if
> they don't need it, but that's LDFLAGS, not CFLAGS.)
> 
> thanks
> -- PMM
Paolo Bonzini Jan. 2, 2021, 7:43 p.m. UTC | #3
On 02/01/21 14:25, Peter Maydell wrote:
> Question to Paolo -- it seems pretty fragile to have to explicitly
> list "these source files need these extra CFLAGS" in half a dozen
> meson.build files, because it's pretty non-obvious that adding
> eg '#include "block/nbd.h"' to a .c file means that you also
> need to update the meson.build file to say "and now it needs these
> extra CFLAGS". Isn't there some way we can just have the CFLAGS
> added more globally so that if we use gnutls.h directly or
> indirectly from more .c files in future it Just Works ?
> 
> If the build failed for the common Linux case then it would be
> at least more obvious that you needed to update the meson.build
> files. I think it's better to avoid "you need to do this special
> thing that you'll only notice you're missing if you happen to test
> on a somewhat obscure host configuration" where we can.
> 
> (We don't want to link helper binaries etc against gnutls if
> they don't need it, but that's LDFLAGS, not CFLAGS.)

The gnutls dependency will already propagate from

if 'CONFIG_GNUTLS' in config_host
   crypto_ss.add(gnutls)
endif

to

libcrypto = static_library('crypto', crypto_ss.sources() + genh,
                           dependencies: [crypto_ss.dependencies()], ...)
crypto = declare_dependency(link_whole: libcrypto,
                             dependencies: [authz, qom])

That is, Meson does know that everything that needs crypto needs gnutls 
(see get_dependencies in mesonbuild/build.py if you're curious).

I think the issue is that dependencies are listed too late---in the 
declare_dependency rather than the static_library.  Take io/ for example:

libio = static_library('io', io_ss.sources() + genh,
                        dependencies: [io_ss.dependencies()],
                        link_with: libqemuutil,
                        name_suffix: 'fa',
                        build_by_default: false)
io = declare_dependency(link_whole: libio, dependencies: [crypto, qom])

Listing "crypto" in io's declare_dependency is enough to propagate the 
gnutls LDFLAGS down to the executables, but it does not add the CFLAGS 
to io/ files itself.  So for the io/ files we aren't telling meson that 
they need crypto (and thus in turn gnutls on the include path).

The fix should be pretty simple and localized to the "Library 
dependencies" section of meson.build.  For the two libraries above, the 
fixed version would look like:

crypto_ss.add(authz, qom)
libcrypto = ... # same as above
crypto = declare_dependency(link_whole: libcrypto)

io_ss.add(crypto, qom)
...
libio = ... # same as above
io = declare_dependency(link_whole: libio)

(Roman, feel free to plunder the above if you want to turn it into a 
commit message, and if it's correct of course).

Thanks,

Paolo
Daniel P. Berrangé Jan. 4, 2021, 12:21 p.m. UTC | #4
On Sat, Jan 02, 2021 at 01:25:07PM +0000, Peter Maydell wrote:
> On Sat, 2 Jan 2021 at 12:54, Roman Bolshakov <r.bolshakov@yadro.com> wrote:
> >
> > crypto/tlscreds.h includes GnuTLS headers if CONFIG_GNUTLS is set, but
> > GNUTLS_CFLAGS, that describe include path, are not propagated
> > transitively to all users of crypto and build fails if GnuTLS headers
> > reside in non-standard directory (which is a case for homebrew on Apple
> > Silicon).
> >
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> 
> Ah, this is https://bugs.launchpad.net/qemu/+bug/1909256
> -- thanks for finding a fix.
> 
> > ---
> >  block/meson.build          | 2 +-
> >  io/meson.build             | 2 +-
> >  meson.build                | 5 +++--
> >  storage-daemon/meson.build | 2 +-
> >  tests/meson.build          | 6 +++---
> >  ui/meson.build             | 2 +-
> >  6 files changed, 10 insertions(+), 9 deletions(-)
> 
> > diff --git a/ui/meson.build b/ui/meson.build
> > index 013258a01c..e6655c94a6 100644
> > --- a/ui/meson.build
> > +++ b/ui/meson.build
> > @@ -29,7 +29,7 @@ vnc_ss.add(files(
> >    'vnc-ws.c',
> >    'vnc-jobs.c',
> >  ))
> > -vnc_ss.add(zlib, png, jpeg)
> > +vnc_ss.add(zlib, png, jpeg, gnutls)
> >  vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
> >  softmmu_ss.add_all(when: vnc, if_true: vnc_ss)
> >  softmmu_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
> 
> Question to Paolo -- it seems pretty fragile to have to explicitly
> list "these source files need these extra CFLAGS" in half a dozen
> meson.build files, because it's pretty non-obvious that adding
> eg '#include "block/nbd.h"' to a .c file means that you also
> need to update the meson.build file to say "and now it needs these
> extra CFLAGS". Isn't there some way we can just have the CFLAGS
> added more globally so that if we use gnutls.h directly or
> indirectly from more .c files in future it Just Works ?

The actual usage of gnutls should be confined to the crypto/ code.

The rest of QEMU should only ever be using QEMU's TLS abstractions
and not directly be tied to GNUTLS. So ideally the gnutls flags
should only ever be added in the crypto/meson.build, and anything
which depends on that should then get the flags indirectly.


Regards,
Daniel
Paolo Bonzini Jan. 4, 2021, 12:30 p.m. UTC | #5
On 04/01/21 13:21, Daniel P. Berrangé wrote:
> The actual usage of gnutls should be confined to the crypto/ code.
> 
> The rest of QEMU should only ever be using QEMU's TLS abstractions
> and not directly be tied to GNUTLS. So ideally the gnutls flags
> should only ever be added in the crypto/meson.build, and anything
> which depends on that should then get the flags indirectly.

Right, see my reply.

Paolo
Peter Maydell Jan. 4, 2021, 1:21 p.m. UTC | #6
On Mon, 4 Jan 2021 at 12:22, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > Question to Paolo -- it seems pretty fragile to have to explicitly
> > list "these source files need these extra CFLAGS" in half a dozen
> > meson.build files, because it's pretty non-obvious that adding
> > eg '#include "block/nbd.h"' to a .c file means that you also
> > need to update the meson.build file to say "and now it needs these
> > extra CFLAGS". Isn't there some way we can just have the CFLAGS
> > added more globally so that if we use gnutls.h directly or
> > indirectly from more .c files in future it Just Works ?
>
> The actual usage of gnutls should be confined to the crypto/ code.
>
> The rest of QEMU should only ever be using QEMU's TLS abstractions
> and not directly be tied to GNUTLS. So ideally the gnutls flags
> should only ever be added in the crypto/meson.build, and anything
> which depends on that should then get the flags indirectly.

Unfortunately include/crypto/tlscreds.h leaks this implementation
detail, because it defines:

struct QCryptoTLSCreds {
    Object parent_obj;
    char *dir;
    QCryptoTLSCredsEndpoint endpoint;
#ifdef CONFIG_GNUTLS
    gnutls_dh_params_t dh_params;
#endif
    bool verifyPeer;
    char *priority;
};

So every file that uses the tlscreds.h header must be
compiled with the GNUTLS flags, even if it doesn't itself
care about the underlying implementation.

(Maybe there's a way to keep the internals of the QCryptoTLSCreds
struct private to the crypto/ source files ?)

thanks
-- PMM
Paolo Bonzini Jan. 4, 2021, 2:40 p.m. UTC | #7
On 04/01/21 14:21, Peter Maydell wrote:
>> The rest of QEMU should only ever be using QEMU's TLS abstractions
>> and not directly be tied to GNUTLS. So ideally the gnutls flags
>> should only ever be added in the crypto/meson.build, and anything
>> which depends on that should then get the flags indirectly.
> Unfortunately include/crypto/tlscreds.h leaks this implementation
> detail

That is not a problem.  As Daniel said, anything depending on crypto can 
still get the gnutls flags _indirectly_.

In my proposed solution to the bug you'd get that via

     io_ss.add(crypto, qom)
     libio = static_library(..., dependencies: io_ss.dependencies())

for example.

Paolo
Peter Maydell Jan. 4, 2021, 3:19 p.m. UTC | #8
On Mon, 4 Jan 2021 at 14:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 04/01/21 14:21, Peter Maydell wrote:
> >> The rest of QEMU should only ever be using QEMU's TLS abstractions
> >> and not directly be tied to GNUTLS. So ideally the gnutls flags
> >> should only ever be added in the crypto/meson.build, and anything
> >> which depends on that should then get the flags indirectly.
> > Unfortunately include/crypto/tlscreds.h leaks this implementation
> > detail
>
> That is not a problem.  As Daniel said, anything depending on crypto can
> still get the gnutls flags _indirectly_.
>
> In my proposed solution to the bug you'd get that via
>
>      io_ss.add(crypto, qom)
>      libio = static_library(..., dependencies: io_ss.dependencies())

Does this work recursively? For instance monitor/qmp-cmds.c
needs the gnutls CFLAGS because:
 * qmp-cmds.c includes ui/vnc.h
 * ui/vnc.h includes include/crypto/tlssession.h
 * include/crypto/tlssession.h includes gnutls.h

I don't see anything in monitor/meson.build that says "qmp-cmds.c
needs whatever ui's dependencies are".

thanks
-- PMM
Roman Bolshakov Jan. 4, 2021, 5:24 p.m. UTC | #9
On Sat, Jan 02, 2021 at 08:43:51PM +0100, Paolo Bonzini wrote:
> On 02/01/21 14:25, Peter Maydell wrote:
> > Question to Paolo -- it seems pretty fragile to have to explicitly
> > list "these source files need these extra CFLAGS" in half a dozen
> > meson.build files, because it's pretty non-obvious that adding
> > eg '#include "block/nbd.h"' to a .c file means that you also
> > need to update the meson.build file to say "and now it needs these
> > extra CFLAGS". Isn't there some way we can just have the CFLAGS
> > added more globally so that if we use gnutls.h directly or
> > indirectly from more .c files in future it Just Works ?
> > 
> > If the build failed for the common Linux case then it would be
> > at least more obvious that you needed to update the meson.build
> > files. I think it's better to avoid "you need to do this special
> > thing that you'll only notice you're missing if you happen to test
> > on a somewhat obscure host configuration" where we can.
> > 
> > (We don't want to link helper binaries etc against gnutls if
> > they don't need it, but that's LDFLAGS, not CFLAGS.)
> 
> The gnutls dependency will already propagate from
> 
> if 'CONFIG_GNUTLS' in config_host
>   crypto_ss.add(gnutls)
> endif
> 
> to
> 
> libcrypto = static_library('crypto', crypto_ss.sources() + genh,
>                           dependencies: [crypto_ss.dependencies()], ...)
> crypto = declare_dependency(link_whole: libcrypto,
>                             dependencies: [authz, qom])
> 

Hi Paolo,

I'm sorry I didn't reply earlier. As I showed in an example to Peter
(https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg00085.html):
https://github.com/mesonbuild/meson/commit/ff5dc65ef841857dd306694dff1fb1cd2bf801e4

The approach doesn't propogate dependencies of crypto beyond libcrypto.
i.e. if you specify crypto somewhere else as depedency, it won't pull
CFLAGS needed for gnutls.

> That is, Meson does know that everything that needs crypto needs gnutls (see
> get_dependencies in mesonbuild/build.py if you're curious).
> 

Thanks. I've been thinking to tinker with it (that's why I made the test case).
Sounds like meson has some issues with transitive dependencies.

> I think the issue is that dependencies are listed too late---in the
> declare_dependency rather than the static_library.  Take io/ for example:
> 
> libio = static_library('io', io_ss.sources() + genh,
>                        dependencies: [io_ss.dependencies()],
>                        link_with: libqemuutil,
>                        name_suffix: 'fa',
>                        build_by_default: false)
> io = declare_dependency(link_whole: libio, dependencies: [crypto, qom])
> 
> Listing "crypto" in io's declare_dependency is enough to propagate the
> gnutls LDFLAGS down to the executables, but it does not add the CFLAGS to
> io/ files itself.  So for the io/ files we aren't telling meson that they
> need crypto (and thus in turn gnutls on the include path).
> 
> The fix should be pretty simple and localized to the "Library dependencies"
> section of meson.build.  For the two libraries above, the fixed version
> would look like:
> 
> crypto_ss.add(authz, qom)
> libcrypto = ... # same as above
> crypto = declare_dependency(link_whole: libcrypto)
> 
> io_ss.add(crypto, qom)
> ...
> libio = ... # same as above
> io = declare_dependency(link_whole: libio)
> 
> (Roman, feel free to plunder the above if you want to turn it into a commit
> message, and if it's correct of course).
> 

Unfortunately it doesn't work, even if crypto is added to io_ss. I think
that's the same issue as in shown in test case above. The patch is
below:

diff --git a/hw/nvram/meson.build b/hw/nvram/meson.build
index fd2951a860..1f2ed013b2 100644
--- a/hw/nvram/meson.build
+++ b/hw/nvram/meson.build
@@ -1,6 +1,3 @@
-# QOM interfaces must be available anytime QOM is used.
-qom_ss.add(files('fw_cfg-interface.c'))
-
 softmmu_ss.add(files('fw_cfg.c'))
 softmmu_ss.add(when: 'CONFIG_CHRP_NVRAM', if_true: files('chrp_nvram.c'))
 softmmu_ss.add(when: 'CONFIG_DS1225Y', if_true: files('ds1225y.c'))
diff --git a/io/meson.build b/io/meson.build
index bcd8b1e737..a844271b17 100644
--- a/io/meson.build
+++ b/io/meson.build
@@ -12,4 +12,4 @@ io_ss.add(files(
   'dns-resolver.c',
   'net-listener.c',
   'task.c',
-))
+), crypto)
diff --git a/meson.build b/meson.build
index 372576f82c..c293ee39e4 100644
--- a/meson.build
+++ b/meson.build
@@ -1538,6 +1538,33 @@ libqemuutil = static_library('qemuutil',
 qemuutil = declare_dependency(link_with: libqemuutil,
                               sources: genh + version_res)
 
+# QOM interfaces must be available anytime QOM is used.
+qom_ss.add(files('hw/nvram/fw_cfg-interface.c'))
+qom_ss = qom_ss.apply(config_host, strict: false)
+libqom = static_library('qom', qom_ss.sources() + genh,
+                        dependencies: [qom_ss.dependencies()],
+                        name_suffix: 'fa')
+
+qom = declare_dependency(link_whole: libqom)
+
+authz_ss = authz_ss.apply(config_host, strict: false)
+libauthz = static_library('authz', authz_ss.sources() + genh,
+                          dependencies: [authz_ss.dependencies()],
+                          name_suffix: 'fa',
+                          build_by_default: false)
+
+authz = declare_dependency(link_whole: libauthz,
+                           dependencies: qom)
+
+crypto_ss = crypto_ss.apply(config_host, strict: false)
+libcrypto = static_library('crypto', crypto_ss.sources() + genh,
+                           dependencies: [crypto_ss.dependencies()],
+                           name_suffix: 'fa',
+                           build_by_default: false)
+
+crypto = declare_dependency(link_whole: libcrypto,
+                            dependencies: [authz, qom])
+
 decodetree = generator(find_program('scripts/decodetree.py'),
                        output: 'decode-@BASENAME@.c.inc',
                        arguments: ['@INPUT@', '@EXTRA_ARGS@', '-o', '@OUTPUT@'])
@@ -1652,31 +1679,6 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms',
                              capture: true,
                              command: [undefsym, nm, '@INPUT@'])
 
-qom_ss = qom_ss.apply(config_host, strict: false)
-libqom = static_library('qom', qom_ss.sources() + genh,
-                        dependencies: [qom_ss.dependencies()],
-                        name_suffix: 'fa')
-
-qom = declare_dependency(link_whole: libqom)
-
-authz_ss = authz_ss.apply(config_host, strict: false)
-libauthz = static_library('authz', authz_ss.sources() + genh,
-                          dependencies: [authz_ss.dependencies()],
-                          name_suffix: 'fa',
-                          build_by_default: false)
-
-authz = declare_dependency(link_whole: libauthz,
-                           dependencies: qom)
-
-crypto_ss = crypto_ss.apply(config_host, strict: false)
-libcrypto = static_library('crypto', crypto_ss.sources() + genh,
-                           dependencies: [crypto_ss.dependencies()],
-                           name_suffix: 'fa',
-                           build_by_default: false)
-
-crypto = declare_dependency(link_whole: libcrypto,
-                            dependencies: [authz, qom])
-
 io_ss = io_ss.apply(config_host, strict: false)
 libio = static_library('io', io_ss.sources() + genh,
                        dependencies: [io_ss.dependencies()],
@@ -1684,7 +1686,7 @@ libio = static_library('io', io_ss.sources() + genh,
                        name_suffix: 'fa',
                        build_by_default: false)
 
-io = declare_dependency(link_whole: libio, dependencies: [crypto, qom])
+io = declare_dependency(link_whole: libio, dependencies: [qom])
 
 libmigration = static_library('migration', sources: migration_files + genh,
                               name_suffix: 'fa',
Paolo Bonzini Jan. 4, 2021, 5:49 p.m. UTC | #10
On 04/01/21 16:19, Peter Maydell wrote:
> On Mon, 4 Jan 2021 at 14:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 04/01/21 14:21, Peter Maydell wrote:
>>>> The rest of QEMU should only ever be using QEMU's TLS abstractions
>>>> and not directly be tied to GNUTLS. So ideally the gnutls flags
>>>> should only ever be added in the crypto/meson.build, and anything
>>>> which depends on that should then get the flags indirectly.
>>> Unfortunately include/crypto/tlscreds.h leaks this implementation
>>> detail
>>
>> That is not a problem.  As Daniel said, anything depending on crypto can
>> still get the gnutls flags _indirectly_.
>>
>> In my proposed solution to the bug you'd get that via
>>
>>       io_ss.add(crypto, qom)
>>       libio = static_library(..., dependencies: io_ss.dependencies())
> 
> Does this work recursively? For instance monitor/qmp-cmds.c
> needs the gnutls CFLAGS because:
>   * qmp-cmds.c includes ui/vnc.h
>   * ui/vnc.h includes include/crypto/tlssession.h
>   * include/crypto/tlssession.h includes gnutls.h
> 
> I don't see anything in monitor/meson.build that says "qmp-cmds.c
> needs whatever ui's dependencies are".

Hmm, I would have thought it would be handled, but Roman said otherwise. 
  I'll look into it, at worst we can fix Meson and temporarily apply 
Roman's patch until it makes it into a release.

Paolo
Peter Maydell Jan. 4, 2021, 5:51 p.m. UTC | #11
On Mon, 4 Jan 2021 at 17:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 04/01/21 16:19, Peter Maydell wrote:
> > Does this work recursively? For instance monitor/qmp-cmds.c
> > needs the gnutls CFLAGS because:
> >   * qmp-cmds.c includes ui/vnc.h
> >   * ui/vnc.h includes include/crypto/tlssession.h
> >   * include/crypto/tlssession.h includes gnutls.h
> >
> > I don't see anything in monitor/meson.build that says "qmp-cmds.c
> > needs whatever ui's dependencies are".
>
> Hmm, I would have thought it would be handled, but Roman said otherwise.
>   I'll look into it, at worst we can fix Meson and temporarily apply
> Roman's patch until it makes it into a release.

NB that for qmp-cmds.c in particular
https://patchew.org/QEMU/20210104161200.15068-1-peter.maydell@linaro.org/
will avoid the accidental inclusion of <gnutls.h>, though I guess
in principle the monitor still needs to say it depends on ui...

thanks
-- PMM
Paolo Bonzini Jan. 4, 2021, 8:50 p.m. UTC | #12
On 04/01/21 18:24, Roman Bolshakov wrote:
> Hi Paolo,
> 
> I'm sorry I didn't reply earlier. As I showed in an example to Peter
> (https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg00085.html):
> https://github.com/mesonbuild/meson/commit/ff5dc65ef841857dd306694dff1fb1cd2bf801e4
> 
> The approach doesn't propogate dependencies of crypto beyond libcrypto.
> i.e. if you specify crypto somewhere else as depedency, it won't pull
> CFLAGS needed for gnutls.

Hi Roman,

After writing the meson patch in fact I noticed that get_dependencies() 
is used only for linker flags.  I got a very quick reply from the Meson 
maintainer (https://github.com/mesonbuild/meson/pull/8151):

    The fact that header flags are not passed transitively but libraries
    are (in some cases) is intentional. Otherwise compiler flag counts
    explode in deep hierarchies. Because of this include paths must be
    exported manually, typically by adding the appropriate bits to a
    declare_dependency.

    Libs are a bit stupid, because you need to add direct dependencies
    if, for example, you link to a static library.

Does it work if you do:

crypto_ss.add(authz, qom)
libcrypto = static_library('crypto', crypto_ss.sources() + genh,
                            dependencies: crypto_ss.dependencies(),
                            ...)
crypto = declare_dependency(link_whole: libcrypto,
                             dependencies: crypto_ss.dependencies())

?  If so, that is also a good option.  If not, I will try to extend the 
test case to pitch the Meson change.

Paolo
Roman Bolshakov Jan. 5, 2021, 2:37 p.m. UTC | #13
On Mon, Jan 04, 2021 at 09:50:32PM +0100, Paolo Bonzini wrote:
> On 04/01/21 18:24, Roman Bolshakov wrote:
> > Hi Paolo,
> > 
> > I'm sorry I didn't reply earlier. As I showed in an example to Peter
> > (https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg00085.html):
> > https://github.com/mesonbuild/meson/commit/ff5dc65ef841857dd306694dff1fb1cd2bf801e4
> > 
> > The approach doesn't propogate dependencies of crypto beyond libcrypto.
> > i.e. if you specify crypto somewhere else as depedency, it won't pull
> > CFLAGS needed for gnutls.
> 
> Hi Roman,
> 
> After writing the meson patch in fact I noticed that get_dependencies() is
> used only for linker flags.  I got a very quick reply from the Meson
> maintainer (https://github.com/mesonbuild/meson/pull/8151):
> 

Thanks for providing a PR! I'll try if it works for QEMU with previous
proposal of fixing it (where we specify dependency in source set only
once and don't duplicate in declare_dependency).

I wonder if we should add a source set method like public_add to allow
the behavior we want and permit propogation of a dependency beyond
static_library without breaking all other users of meson out there?

>    The fact that header flags are not passed transitively but libraries
>    are (in some cases) is intentional. Otherwise compiler flag counts
>    explode in deep hierarchies. Because of this include paths must be
>    exported manually, typically by adding the appropriate bits to a
>    declare_dependency.
> 
>    Libs are a bit stupid, because you need to add direct dependencies
>    if, for example, you link to a static library.
> 
> Does it work if you do:
> 
> crypto_ss.add(authz, qom)
> libcrypto = static_library('crypto', crypto_ss.sources() + genh,
>                            dependencies: crypto_ss.dependencies(),
>                            ...)
> crypto = declare_dependency(link_whole: libcrypto,
>                             dependencies: crypto_ss.dependencies())
> 

I tried that approach before I sent the patch in the subject. It
produces duplicate symbols:

  duplicate symbol '_qauthz_pam_new' in:
      libcrypto.fa(authz_pamacct.c.o)
      libauthz.fa(authz_pamacct.c.o)
  [...]
  duplicate symbol '_object_property_set_qobject' in:
      libcrypto.fa(qom_qom-qobject.c.o)                                                                                                                                                                                                                            libqom.fa(qom_qom-qobject.c.o)

My impression that it links in every static library that's mentioned in
dependencies of static_library, so they grow like a snow ball. Patch
below:

diff --git a/block/meson.build b/block/meson.build
index 7595d86c41..7eaf48c6dc 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -40,7 +40,7 @@ block_ss.add(files(
   'vmdk.c',
   'vpc.c',
   'write-threshold.c',
-), zstd, zlib)
+), crypto, zstd, zlib)
 
 softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('blkreplay.c'))
 
diff --git a/hw/nvram/meson.build b/hw/nvram/meson.build
index fd2951a860..1f2ed013b2 100644
--- a/hw/nvram/meson.build
+++ b/hw/nvram/meson.build
@@ -1,6 +1,3 @@
-# QOM interfaces must be available anytime QOM is used.
-qom_ss.add(files('fw_cfg-interface.c'))
-
 softmmu_ss.add(files('fw_cfg.c'))
 softmmu_ss.add(when: 'CONFIG_CHRP_NVRAM', if_true: files('chrp_nvram.c'))
 softmmu_ss.add(when: 'CONFIG_DS1225Y', if_true: files('ds1225y.c'))
diff --git a/io/meson.build b/io/meson.build
index bcd8b1e737..a844271b17 100644
--- a/io/meson.build
+++ b/io/meson.build
@@ -12,4 +12,4 @@ io_ss.add(files(
   'dns-resolver.c',
   'net-listener.c',
   'task.c',
-))
+), crypto)
diff --git a/meson.build b/meson.build
index 372576f82c..1a8c653067 100644
--- a/meson.build
+++ b/meson.build
@@ -1538,6 +1538,34 @@ libqemuutil = static_library('qemuutil',
 qemuutil = declare_dependency(link_with: libqemuutil,
                               sources: genh + version_res)
 
+# QOM interfaces must be available anytime QOM is used.
+qom_ss.add(files('hw/nvram/fw_cfg-interface.c'))
+qom_ss = qom_ss.apply(config_host, strict: false)
+libqom = static_library('qom', qom_ss.sources() + genh,
+                        dependencies: [qom_ss.dependencies()],
+                        name_suffix: 'fa')
+
+qom = declare_dependency(link_whole: libqom)
+
+authz_ss = authz_ss.apply(config_host, strict: false)
+libauthz = static_library('authz', authz_ss.sources() + genh,
+                          dependencies: [authz_ss.dependencies()],
+                          name_suffix: 'fa',
+                          build_by_default: false)
+
+authz = declare_dependency(link_whole: libauthz,
+                           dependencies: qom)
+
+crypto_ss.add(authz)
+crypto_ss = crypto_ss.apply(config_host, strict: false)
+libcrypto = static_library('crypto', crypto_ss.sources() + genh,
+                           dependencies: crypto_ss.dependencies(),
+                           name_suffix: 'fa',
+                           build_by_default: false)
+
+crypto = declare_dependency(link_whole: libcrypto,
+                            dependencies: crypto_ss.dependencies())
+
 decodetree = generator(find_program('scripts/decodetree.py'),
                        output: 'decode-@BASENAME@.c.inc',
                        arguments: ['@INPUT@', '@EXTRA_ARGS@', '-o', '@OUTPUT@'])
@@ -1652,31 +1680,6 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms',
                              capture: true,
                              command: [undefsym, nm, '@INPUT@'])
 
-qom_ss = qom_ss.apply(config_host, strict: false)
-libqom = static_library('qom', qom_ss.sources() + genh,
-                        dependencies: [qom_ss.dependencies()],
-                        name_suffix: 'fa')
-
-qom = declare_dependency(link_whole: libqom)
-
-authz_ss = authz_ss.apply(config_host, strict: false)
-libauthz = static_library('authz', authz_ss.sources() + genh,
-                          dependencies: [authz_ss.dependencies()],
-                          name_suffix: 'fa',
-                          build_by_default: false)
-
-authz = declare_dependency(link_whole: libauthz,
-                           dependencies: qom)
-
-crypto_ss = crypto_ss.apply(config_host, strict: false)
-libcrypto = static_library('crypto', crypto_ss.sources() + genh,
-                           dependencies: [crypto_ss.dependencies()],
-                           name_suffix: 'fa',
-                           build_by_default: false)
-
-crypto = declare_dependency(link_whole: libcrypto,
-                            dependencies: [authz, qom])
-
 io_ss = io_ss.apply(config_host, strict: false)
 libio = static_library('io', io_ss.sources() + genh,
                        dependencies: [io_ss.dependencies()],

> ?  If so, that is also a good option.  If not, I will try to extend the test
> case to pitch the Meson change.
> 

This one seems the only sane approach left.

Thanks,
Roman
Paolo Bonzini Jan. 5, 2021, 5:41 p.m. UTC | #14
On 05/01/21 15:37, Roman Bolshakov wrote:
>> Does it work if you do:
>>
>> crypto_ss.add(authz, qom)
>> libcrypto = static_library('crypto', crypto_ss.sources() + genh,
>>                             dependencies: crypto_ss.dependencies(),
>>                             ...)
>> crypto = declare_dependency(link_whole: libcrypto,
>>                              dependencies: crypto_ss.dependencies())
>>
> 
> I tried that approach before I sent the patch in the subject. It
> produces duplicate symbols:
> 
>    duplicate symbol '_qauthz_pam_new' in:
>        libcrypto.fa(authz_pamacct.c.o)
>        libauthz.fa(authz_pamacct.c.o)
>    [...]
>    duplicate symbol '_object_property_set_qobject' in:
>        libcrypto.fa(qom_qom-qobject.c.o)                                                                                                                                                                                                                            libqom.fa(qom_qom-qobject.c.o)
> 
> My impression that it links in every static library that's mentioned in
> dependencies of static_library, so they grow like a snow ball. Patch
> below:

Okay, I'll look more into it.

Paolo

> diff --git a/block/meson.build b/block/meson.build
> index 7595d86c41..7eaf48c6dc 100644
> --- a/block/meson.build
> +++ b/block/meson.build
> @@ -40,7 +40,7 @@ block_ss.add(files(
>     'vmdk.c',
>     'vpc.c',
>     'write-threshold.c',
> -), zstd, zlib)
> +), crypto, zstd, zlib)
>   
>   softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('blkreplay.c'))
>   
> diff --git a/hw/nvram/meson.build b/hw/nvram/meson.build
> index fd2951a860..1f2ed013b2 100644
> --- a/hw/nvram/meson.build
> +++ b/hw/nvram/meson.build
> @@ -1,6 +1,3 @@
> -# QOM interfaces must be available anytime QOM is used.
> -qom_ss.add(files('fw_cfg-interface.c'))
> -
>   softmmu_ss.add(files('fw_cfg.c'))
>   softmmu_ss.add(when: 'CONFIG_CHRP_NVRAM', if_true: files('chrp_nvram.c'))
>   softmmu_ss.add(when: 'CONFIG_DS1225Y', if_true: files('ds1225y.c'))
> diff --git a/io/meson.build b/io/meson.build
> index bcd8b1e737..a844271b17 100644
> --- a/io/meson.build
> +++ b/io/meson.build
> @@ -12,4 +12,4 @@ io_ss.add(files(
>     'dns-resolver.c',
>     'net-listener.c',
>     'task.c',
> -))
> +), crypto)
> diff --git a/meson.build b/meson.build
> index 372576f82c..1a8c653067 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1538,6 +1538,34 @@ libqemuutil = static_library('qemuutil',
>   qemuutil = declare_dependency(link_with: libqemuutil,
>                                 sources: genh + version_res)
>   
> +# QOM interfaces must be available anytime QOM is used.
> +qom_ss.add(files('hw/nvram/fw_cfg-interface.c'))
> +qom_ss = qom_ss.apply(config_host, strict: false)
> +libqom = static_library('qom', qom_ss.sources() + genh,
> +                        dependencies: [qom_ss.dependencies()],
> +                        name_suffix: 'fa')
> +
> +qom = declare_dependency(link_whole: libqom)
> +
> +authz_ss = authz_ss.apply(config_host, strict: false)
> +libauthz = static_library('authz', authz_ss.sources() + genh,
> +                          dependencies: [authz_ss.dependencies()],
> +                          name_suffix: 'fa',
> +                          build_by_default: false)
> +
> +authz = declare_dependency(link_whole: libauthz,
> +                           dependencies: qom)
> +
> +crypto_ss.add(authz)
> +crypto_ss = crypto_ss.apply(config_host, strict: false)
> +libcrypto = static_library('crypto', crypto_ss.sources() + genh,
> +                           dependencies: crypto_ss.dependencies(),
> +                           name_suffix: 'fa',
> +                           build_by_default: false)
> +
> +crypto = declare_dependency(link_whole: libcrypto,
> +                            dependencies: crypto_ss.dependencies())
> +
>   decodetree = generator(find_program('scripts/decodetree.py'),
>                          output: 'decode-@BASENAME@.c.inc',
>                          arguments: ['@INPUT@', '@EXTRA_ARGS@', '-o', '@OUTPUT@'])
> @@ -1652,31 +1680,6 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms',
>                                capture: true,
>                                command: [undefsym, nm, '@INPUT@'])
>   
> -qom_ss = qom_ss.apply(config_host, strict: false)
> -libqom = static_library('qom', qom_ss.sources() + genh,
> -                        dependencies: [qom_ss.dependencies()],
> -                        name_suffix: 'fa')
> -
> -qom = declare_dependency(link_whole: libqom)
> -
> -authz_ss = authz_ss.apply(config_host, strict: false)
> -libauthz = static_library('authz', authz_ss.sources() + genh,
> -                          dependencies: [authz_ss.dependencies()],
> -                          name_suffix: 'fa',
> -                          build_by_default: false)
> -
> -authz = declare_dependency(link_whole: libauthz,
> -                           dependencies: qom)
> -
> -crypto_ss = crypto_ss.apply(config_host, strict: false)
> -libcrypto = static_library('crypto', crypto_ss.sources() + genh,
> -                           dependencies: [crypto_ss.dependencies()],
> -                           name_suffix: 'fa',
> -                           build_by_default: false)
> -
> -crypto = declare_dependency(link_whole: libcrypto,
> -                            dependencies: [authz, qom])
> -
>   io_ss = io_ss.apply(config_host, strict: false)
>   libio = static_library('io', io_ss.sources() + genh,
>                          dependencies: [io_ss.dependencies()],
Paolo Bonzini Jan. 7, 2021, 11:41 a.m. UTC | #15
On 05/01/21 15:37, Roman Bolshakov wrote:
> Does it work if you do:
> 
> crypto_ss.add(authz, qom)
> libcrypto = static_library('crypto', crypto_ss.sources() + genh,
>                             dependencies: crypto_ss.dependencies(),
>                             ...)
> crypto = declare_dependency(link_whole: libcrypto,
>                              dependencies: crypto_ss.dependencies())

Ok, so the final attempt is a mix of the three :)  Keep the link_whole 
dependencies in the declare_dependency, and add the sourceset 
dependencies there too.

diff --git a/meson.build b/meson.build
index e9bf290966..774df4db8e 100644
--- a/meson.build
+++ b/meson.build
@@ -1904,7 +1904,8 @@ libqom = static_library('qom', qom_ss.sources() + 
genh,
                          dependencies: [qom_ss.dependencies()],
                          name_suffix: 'fa')

-qom = declare_dependency(link_whole: libqom)
+qom = declare_dependency(link_whole: libqom,
+                         dependencies: [qom_ss.dependencies()])

  authz_ss = authz_ss.apply(config_host, strict: false)
  libauthz = static_library('authz', authz_ss.sources() + genh,
@@ -1913,7 +1914,7 @@ libauthz = static_library('authz', 
authz_ss.sources() + genh,
                            build_by_default: false)

  authz = declare_dependency(link_whole: libauthz,
-                           dependencies: qom)
+                          dependencies: [authz_ss.dependencies(), qom])

  crypto_ss = crypto_ss.apply(config_host, strict: false)
  libcrypto = static_library('crypto', crypto_ss.sources() + genh,
@@ -1922,7 +1923,7 @@ libcrypto = static_library('crypto', 
crypto_ss.sources() + genh,
                             build_by_default: false)

  crypto = declare_dependency(link_whole: libcrypto,
-                            dependencies: [authz, qom])
+                            dependencies: [crypto_ss.dependencies(), 
authz, qom])

  io_ss = io_ss.apply(config_host, strict: false)
  libio = static_library('io', io_ss.sources() + genh,
@@ -1931,13 +1932,14 @@ libio = static_library('io', io_ss.sources() + genh,
                         name_suffix: 'fa',
                         build_by_default: false)

-io = declare_dependency(link_whole: libio, dependencies: [crypto, qom])
+io = declare_dependency(link_whole: libio,
+                        dependencies: [io_ss.dependencies(), crypto, qom])

  libmigration = static_library('migration', sources: migration_files + 
genh,
                                name_suffix: 'fa',
                                build_by_default: false)
  migration = declare_dependency(link_with: libmigration,
-                               dependencies: [zlib, qom, io])
+                               dependencies: [qom, io])
  softmmu_ss.add(migration)

  block_ss = block_ss.apply(config_host, strict: false)
@@ -1949,7 +1951,7 @@ libblock = static_library('block', 
block_ss.sources() + genh,

  block = declare_dependency(link_whole: [libblock],
                             link_args: '@block.syms',
-                           dependencies: [crypto, io])
+                           dependencies: [block_ss.dependencies(), 
crypto, io])

  blockdev_ss = blockdev_ss.apply(config_host, strict: false)
  libblockdev = static_library('blockdev', blockdev_ss.sources() + genh,
@@ -1958,7 +1960,7 @@ libblockdev = static_library('blockdev', 
blockdev_ss.sources() + genh,
                               build_by_default: false)

  blockdev = declare_dependency(link_whole: [libblockdev],
-                              dependencies: [block])
+                              dependencies: 
[blockdev_ss.dependencies(), block])

  qmp_ss = qmp_ss.apply(config_host, strict: false)
  libqmp = static_library('qmp', qmp_ss.sources() + genh,
@@ -1966,7 +1968,8 @@ libqmp = static_library('qmp', qmp_ss.sources() + 
genh,
                          name_suffix: 'fa',
                          build_by_default: false)

-qmp = declare_dependency(link_whole: [libqmp])
+qmp = declare_dependency(link_whole: [libqmp],
+                         dependencies: qmp_ss.dependencies())

  libchardev = static_library('chardev', chardev_ss.sources() + genh,
                              name_suffix: 'fa',
diff --git a/migration/meson.build b/migration/meson.build
index 9645f44005..e1f237b5db 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -9,6 +9,7 @@ migration_files = files(
  )
  softmmu_ss.add(migration_files)

+softmmu_ss.add(zlib)
  softmmu_ss.add(files(
    'block-dirty-bitmap.c',
    'channel.c',
Roman Bolshakov Jan. 7, 2021, 3:56 p.m. UTC | #16
On Thu, Jan 07, 2021 at 12:41:40PM +0100, Paolo Bonzini wrote:
> On 05/01/21 15:37, Roman Bolshakov wrote:
> > Does it work if you do:
> > 
> > crypto_ss.add(authz, qom)
> > libcrypto = static_library('crypto', crypto_ss.sources() + genh,
> >                             dependencies: crypto_ss.dependencies(),
> >                             ...)
> > crypto = declare_dependency(link_whole: libcrypto,
> >                              dependencies: crypto_ss.dependencies())
> 
> Ok, so the final attempt is a mix of the three :)  Keep the link_whole
> dependencies in the declare_dependency, and add the sourceset dependencies
> there too.

Hi Paolo,

Thanks for the patch but unfortunately it doesn't resolve the issue.
io and other libraries can't still find gnutls.

I've also tried your meson trans-deps branch and wonder if it's supposed
to fix the issue without any changes to qemu build files?
Do you need any help with meson changes?

IMO duplication of dependencies shouldn't be needed for a build system.
Meta build system should allow private and public dependencies. Different
rules are applied to them. Private dependency is not propagated beyond a
target that uses it, public dependency is propagated. There's also
declare_dependency that has to be always public because it serves no
purpose on it's own. declare_dependency is like INTERFACE library in
CMake.

If a project specifies a dependency that is public, it should be
transitively passed downstream. Build system shouldn't obscurely hide
flags a dependency provides on case-by-case basis.

Right now it seems that meson is missing the notion of public and
private dependencies and that's where the problem arises. The post [1] (and
the related issue) summarizes what I'm trying to say.

If we resolve the issue, then we just specify gnutls as a public
dependency of crypto and all users of crypto would get gnutls headers.

Here's an example how clearly CMake approaches the issue [2][3]:

add_library(crypto OBJECT crypto-file1.c ...)
target_link_libraries(crypto PRIVATE aninternaldep
                             PUBLIC  gnutls
                                     anotherpublicdep)

1. https://github.com/mesonbuild/meson/issues/495#issuecomment-206178570
2. https://cmake.org/cmake/help/latest/command/target_link_libraries.html#linking-object-libraries
3. https://cmake.org/cmake/help/latest/command/target_link_libraries.html#libraries-for-a-target-and-or-its-dependents

Regards,
Roman

> 
> diff --git a/meson.build b/meson.build
> index e9bf290966..774df4db8e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1904,7 +1904,8 @@ libqom = static_library('qom', qom_ss.sources() +
> genh,
>                          dependencies: [qom_ss.dependencies()],
>                          name_suffix: 'fa')
> 
> -qom = declare_dependency(link_whole: libqom)
> +qom = declare_dependency(link_whole: libqom,
> +                         dependencies: [qom_ss.dependencies()])
> 
>  authz_ss = authz_ss.apply(config_host, strict: false)
>  libauthz = static_library('authz', authz_ss.sources() + genh,
> @@ -1913,7 +1914,7 @@ libauthz = static_library('authz', authz_ss.sources()
> + genh,
>                            build_by_default: false)
> 
>  authz = declare_dependency(link_whole: libauthz,
> -                           dependencies: qom)
> +                          dependencies: [authz_ss.dependencies(), qom])
> 
>  crypto_ss = crypto_ss.apply(config_host, strict: false)
>  libcrypto = static_library('crypto', crypto_ss.sources() + genh,
> @@ -1922,7 +1923,7 @@ libcrypto = static_library('crypto',
> crypto_ss.sources() + genh,
>                             build_by_default: false)
> 
>  crypto = declare_dependency(link_whole: libcrypto,
> -                            dependencies: [authz, qom])
> +                            dependencies: [crypto_ss.dependencies(), authz,
> qom])
> 
>  io_ss = io_ss.apply(config_host, strict: false)
>  libio = static_library('io', io_ss.sources() + genh,
> @@ -1931,13 +1932,14 @@ libio = static_library('io', io_ss.sources() + genh,
>                         name_suffix: 'fa',
>                         build_by_default: false)
> 
> -io = declare_dependency(link_whole: libio, dependencies: [crypto, qom])
> +io = declare_dependency(link_whole: libio,
> +                        dependencies: [io_ss.dependencies(), crypto, qom])
> 
>  libmigration = static_library('migration', sources: migration_files + genh,
>                                name_suffix: 'fa',
>                                build_by_default: false)
>  migration = declare_dependency(link_with: libmigration,
> -                               dependencies: [zlib, qom, io])
> +                               dependencies: [qom, io])
>  softmmu_ss.add(migration)
> 
>  block_ss = block_ss.apply(config_host, strict: false)
> @@ -1949,7 +1951,7 @@ libblock = static_library('block', block_ss.sources()
> + genh,
> 
>  block = declare_dependency(link_whole: [libblock],
>                             link_args: '@block.syms',
> -                           dependencies: [crypto, io])
> +                           dependencies: [block_ss.dependencies(), crypto,
> io])
> 
>  blockdev_ss = blockdev_ss.apply(config_host, strict: false)
>  libblockdev = static_library('blockdev', blockdev_ss.sources() + genh,
> @@ -1958,7 +1960,7 @@ libblockdev = static_library('blockdev',
> blockdev_ss.sources() + genh,
>                               build_by_default: false)
> 
>  blockdev = declare_dependency(link_whole: [libblockdev],
> -                              dependencies: [block])
> +                              dependencies: [blockdev_ss.dependencies(),
> block])
> 
>  qmp_ss = qmp_ss.apply(config_host, strict: false)
>  libqmp = static_library('qmp', qmp_ss.sources() + genh,
> @@ -1966,7 +1968,8 @@ libqmp = static_library('qmp', qmp_ss.sources() +
> genh,
>                          name_suffix: 'fa',
>                          build_by_default: false)
> 
> -qmp = declare_dependency(link_whole: [libqmp])
> +qmp = declare_dependency(link_whole: [libqmp],
> +                         dependencies: qmp_ss.dependencies())
> 
>  libchardev = static_library('chardev', chardev_ss.sources() + genh,
>                              name_suffix: 'fa',
> diff --git a/migration/meson.build b/migration/meson.build
> index 9645f44005..e1f237b5db 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -9,6 +9,7 @@ migration_files = files(
>  )
>  softmmu_ss.add(migration_files)
> 
> +softmmu_ss.add(zlib)
>  softmmu_ss.add(files(
>    'block-dirty-bitmap.c',
>    'channel.c',
>
Paolo Bonzini Jan. 7, 2021, 4:23 p.m. UTC | #17
On 07/01/21 16:56, Roman Bolshakov wrote:
> IMO duplication of dependencies shouldn't be needed for a build system.
> Meta build system should allow private and public dependencies. Different
> rules are applied to them. Private dependency is not propagated beyond a
> target that uses it, public dependency is propagated.
> 
> Right now it seems that meson is missing the notion of public and
> private dependencies and that's where the problem arises. The post [1] (and
> the related issue) summarizes what I'm trying to say.

Meson doesn't have a concept of public dependencies because it separates 
the private (static library) and the public (declare_dependency) view. 
That is you'd have:

public_deps = [gnutls, anotherpublicdep]
lib = static_library(..., dependencies: public_deps + [aninternaldep])
dep = declare_dependency(link_with: lib, dependencies: public_deps)

The real issue is that Meson's implementation of link_whole for 
library-in-library makes sense for one use case (convenience library 
that is linked into another convenience library) but not for another 
(grouping code for subsystems).  I cannot blame them for this because 
link_with is a more common case for the latter; OTOH QEMU is using 
link_whole a lot in order to support the *_init() construct.

I really think the correct fix is for Meson to use objects instead of 
archives for link_whole, similar to how QEMU Makefiles used to do it. 
This would also remove the need for the special .fa suffix, so it would 
be an improvement all around.

Paolo

> If we resolve the issue, then we just specify gnutls as a public
> dependency of crypto and all users of crypto would get gnutls headers.
> 
> Here's an example how clearly CMake approaches the issue [2][3]:
> 
> add_library(crypto OBJECT crypto-file1.c ...)
> target_link_libraries(crypto PRIVATE aninternaldep
>                               PUBLIC  gnutls
>                                       anotherpublicdep)
> 
> 1.https://github.com/mesonbuild/meson/issues/495#issuecomment-206178570
> 2.https://cmake.org/cmake/help/latest/command/target_link_libraries.html#linking-object-libraries
> 3.https://cmake.org/cmake/help/latest/command/target_link_libraries.html#libraries-for-a-target-and-or-its-dependents
Roman Bolshakov Jan. 7, 2021, 6:18 p.m. UTC | #18
On Thu, Jan 07, 2021 at 05:23:54PM +0100, Paolo Bonzini wrote:
> On 07/01/21 16:56, Roman Bolshakov wrote:
> > IMO duplication of dependencies shouldn't be needed for a build system.
> > Meta build system should allow private and public dependencies. Different
> > rules are applied to them. Private dependency is not propagated beyond a
> > target that uses it, public dependency is propagated.
> > 
> > Right now it seems that meson is missing the notion of public and
> > private dependencies and that's where the problem arises. The post [1] (and
> > the related issue) summarizes what I'm trying to say.
> 
> Meson doesn't have a concept of public dependencies because it separates the
> private (static library) and the public (declare_dependency) view. That is
> you'd have:
> 
> public_deps = [gnutls, anotherpublicdep]
> lib = static_library(..., dependencies: public_deps + [aninternaldep])
> dep = declare_dependency(link_with: lib, dependencies: public_deps)
> 

Thanks! This wasn't obvious to me. But what's not clear that CMake can
do both collection of objects (what I provided in the example) and
static libraries and they're different. I assume what you have shown
would look in CMake like (please note that STATIC is used instead of
OBJECT):

add_library(crypto STATIC crypto-file1.c ...)
target_link_libraries(crypto PRIVATE aninternaldep
                              PUBLIC  gnutls
                                      anotherpublicdep)


That explains why attempt to use dependencies between link_whole static
libraries in meson causes symbol duplication. CMake on other hand can
just make collection of objects or even a chain of collection of
objects. They'll be linked in fully only in a final static library,
shared library or an executable.

> The real issue is that Meson's implementation of link_whole for
> library-in-library makes sense for one use case (convenience library that is
> linked into another convenience library) but not for another (grouping code
> for subsystems).  I cannot blame them for this because link_with is a more
> common case for the latter; OTOH QEMU is using link_whole a lot in order to
> support the *_init() construct.
> 
> I really think the correct fix is for Meson to use objects instead of
> archives for link_whole, similar to how QEMU Makefiles used to do it. This
> would also remove the need for the special .fa suffix, so it would be an
> improvement all around.
> 

Does it mean that we need a kind of object target in meson? Do you think
if this interface would work?

crypto_objs = object_library(..., dependencies: public_deps + [aninternaldep])
crypto = declare_dependency(link_with: crypto_objs, dependencies: public_deps)

Regards,
Roman

> Paolo
> 
> > If we resolve the issue, then we just specify gnutls as a public
> > dependency of crypto and all users of crypto would get gnutls headers.
> > 
> > Here's an example how clearly CMake approaches the issue [2][3]:
> > 
> > add_library(crypto OBJECT crypto-file1.c ...)
> > target_link_libraries(crypto PRIVATE aninternaldep
> >                               PUBLIC  gnutls
> >                                       anotherpublicdep)
> > 
> > 1.https://github.com/mesonbuild/meson/issues/495#issuecomment-206178570
> > 2.https://cmake.org/cmake/help/latest/command/target_link_libraries.html#linking-object-libraries
> > 3.https://cmake.org/cmake/help/latest/command/target_link_libraries.html#libraries-for-a-target-and-or-its-dependents
>
Paolo Bonzini Jan. 7, 2021, 6:22 p.m. UTC | #19
On 07/01/21 19:18, Roman Bolshakov wrote:
> 
>> The real issue is that Meson's implementation of link_whole for
>> library-in-library makes sense for one use case (convenience library that is
>> linked into another convenience library) but not for another (grouping code
>> for subsystems).  I cannot blame them for this because link_with is a more
>> common case for the latter; OTOH QEMU is using link_whole a lot in order to
>> support the *_init() construct.
>>
>> I really think the correct fix is for Meson to use objects instead of
>> archives for link_whole, similar to how QEMU Makefiles used to do it. This
>> would also remove the need for the special .fa suffix, so it would be an
>> improvement all around.
>>
> Does it mean that we need a kind of object target in meson? Do you think
> if this interface would work?
> 
> crypto_objs = object_library(..., dependencies: public_deps + [aninternaldep])
> crypto = declare_dependency(link_with: crypto_objs, dependencies: public_deps)

No I think that Meson should simply explode link_whole libraries to 
their constituent objects.  This way duplicates are avoided.

Paolo
Roman Bolshakov Jan. 7, 2021, 7:36 p.m. UTC | #20
On Thu, Jan 07, 2021 at 07:22:06PM +0100, Paolo Bonzini wrote:
> On 07/01/21 19:18, Roman Bolshakov wrote:
> > 
> > > The real issue is that Meson's implementation of link_whole for
> > > library-in-library makes sense for one use case (convenience library that is
> > > linked into another convenience library) but not for another (grouping code
> > > for subsystems).  I cannot blame them for this because link_with is a more
> > > common case for the latter; OTOH QEMU is using link_whole a lot in order to
> > > support the *_init() construct.
> > > 
> > > I really think the correct fix is for Meson to use objects instead of
> > > archives for link_whole, similar to how QEMU Makefiles used to do it. This
> > > would also remove the need for the special .fa suffix, so it would be an
> > > improvement all around.
> > > 
> > Does it mean that we need a kind of object target in meson? Do you think
> > if this interface would work?
> > 
> > crypto_objs = object_library(..., dependencies: public_deps + [aninternaldep])
> > crypto = declare_dependency(link_with: crypto_objs, dependencies: public_deps)
> 
> No I think that Meson should simply explode link_whole libraries to their
> constituent objects.  This way duplicates are avoided.
> 

Ok. I've looked through related changes in meson and it flattens object
files implicitly for link_with/link_whole parameters of static_library:

  https://github.com/mesonbuild/meson/pull/6030/files

But qemu adds dependencies to source set and populates dependencies
parameter of static_library and declare_dependency and we get duplicate
symbols:

  https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg00411.html

Perhaps it's a bug then.

Regards,
Roman
Paolo Bonzini Jan. 7, 2021, 7:41 p.m. UTC | #21
Il gio 7 gen 2021, 20:36 Roman Bolshakov <r.bolshakov@yadro.com> ha scritto:

> > No I think that Meson should simply explode link_whole libraries to their
> > constituent objects.  This way duplicates are avoided.
> >
>
> Ok. I've looked through related changes in meson and it flattens object
> files implicitly for link_with/link_whole parameters of static_library:
>
>   https://github.com/mesonbuild/meson/pull/6030/files
>
> But qemu adds dependencies to source set and populates dependencies
> parameter of static_library and declare_dependency and we get duplicate
> symbols:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg00411.html
>
> Perhaps it's a bug then.
>

No, the same deduplication is not done for executables, because executables
use libraries directly and not their object files.

Paolo

>
> Regards,
> Roman
>
>
Roman Bolshakov Jan. 8, 2021, 7:29 p.m. UTC | #22
On Thu, Jan 07, 2021 at 08:41:50PM +0100, Paolo Bonzini wrote:
> Il gio 7 gen 2021, 20:36 Roman Bolshakov <r.bolshakov@yadro.com> ha scritto:
> 
> > > No I think that Meson should simply explode link_whole libraries to their
> > > constituent objects.  This way duplicates are avoided.
> > >
> >
> > Ok. I've looked through related changes in meson and it flattens object
> > files implicitly for link_with/link_whole parameters of static_library:
> >
> >   https://github.com/mesonbuild/meson/pull/6030/files
> >
> > But qemu adds dependencies to source set and populates dependencies
> > parameter of static_library and declare_dependency and we get duplicate
> > symbols:
> >
> >   https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg00411.html
> >
> > Perhaps it's a bug then.
> >
> 
> No, the same deduplication is not done for executables, because executables
> use libraries directly and not their object files.
> 

Paolo,

I tried to use extract_all_objects() to get all object files directly
but it doesn't work on dependency objects defined via
declare_dependency(). It works only on regular targets (libs and
executables). And as far as I understand the intention to have
declare_dependency() in QEMU was to specify public interface to avoid
some duplication. But meson doesn't have public/private notion for build
targets so if we drop declare_dependency we need to specify link_whole
in every user of a library that's had link_whole: declare_dependency()
and build files would become less lean. So I'm not sure how to proceed.

The proposed patch (in the subject) is the still the best we've got so
far that fixes macOS build immediately without much bigger wrestling
with meson.

-Roman
Paolo Bonzini Jan. 8, 2021, 8:02 p.m. UTC | #23
On 08/01/21 20:29, Roman Bolshakov wrote:
> Paolo,
> 
> I tried to use extract_all_objects() to get all object files directly
> but it doesn't work on dependency objects defined via
> declare_dependency(). It works only on regular targets (libs and
> executables). And as far as I understand the intention to have
> declare_dependency() in QEMU was to specify public interface to avoid
> some duplication. But meson doesn't have public/private notion for build
> targets so if we drop declare_dependency we need to specify link_whole
> in every user of a library that's had link_whole: declare_dependency()
> and build files would become less lean. So I'm not sure how to proceed.

Yes, that was just saying that the code was _in Meson_ but it still 
needs a change to the ninja backend.

> The proposed patch (in the subject) is the still the best we've got so
> far that fixes macOS build immediately without much bigger wrestling
> with meson.

Yes, I'm going to queue it.

Paolo
diff mbox series

Patch

diff --git a/block/meson.build b/block/meson.build
index 5dcc1e5cce..61fc5e5955 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -39,7 +39,7 @@  block_ss.add(files(
   'vmdk.c',
   'vpc.c',
   'write-threshold.c',
-), zstd, zlib)
+), zstd, zlib, gnutls)
 
 softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('blkreplay.c'))
 
diff --git a/io/meson.build b/io/meson.build
index bcd8b1e737..bbcd3c53a4 100644
--- a/io/meson.build
+++ b/io/meson.build
@@ -12,4 +12,4 @@  io_ss.add(files(
   'dns-resolver.c',
   'net-listener.c',
   'task.c',
-))
+), gnutls)
diff --git a/meson.build b/meson.build
index 372576f82c..d39fc018f4 100644
--- a/meson.build
+++ b/meson.build
@@ -1567,7 +1567,7 @@  blockdev_ss.add(files(
   'blockdev-nbd.c',
   'iothread.c',
   'job-qmp.c',
-))
+), gnutls)
 
 # os-posix.c contains POSIX-specific functions used by qemu-storage-daemon,
 # os-win32.c does not
@@ -1723,6 +1723,7 @@  qmp = declare_dependency(link_whole: [libqmp])
 
 libchardev = static_library('chardev', chardev_ss.sources() + genh,
                             name_suffix: 'fa',
+                            dependencies: [gnutls],
                             build_by_default: false)
 
 chardev = declare_dependency(link_whole: libchardev)
@@ -1941,7 +1942,7 @@  if have_tools
   qemu_io = executable('qemu-io', files('qemu-io.c'),
              dependencies: [block, qemuutil], install: true)
   qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
-               dependencies: [blockdev, qemuutil], install: true)
+               dependencies: [blockdev, qemuutil, gnutls], install: true)
 
   subdir('storage-daemon')
   subdir('contrib/rdmacm-mux')
diff --git a/storage-daemon/meson.build b/storage-daemon/meson.build
index c5adce81c3..68852f3d25 100644
--- a/storage-daemon/meson.build
+++ b/storage-daemon/meson.build
@@ -1,6 +1,6 @@ 
 qsd_ss = ss.source_set()
 qsd_ss.add(files('qemu-storage-daemon.c'))
-qsd_ss.add(blockdev, chardev, qmp, qom, qemuutil)
+qsd_ss.add(blockdev, chardev, qmp, qom, qemuutil, gnutls)
 
 subdir('qapi')
 
diff --git a/tests/meson.build b/tests/meson.build
index 1fa068f27b..29ebaba48d 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -159,11 +159,11 @@  if have_block
      'CONFIG_POSIX' in config_host
     tests += {
       'test-crypto-tlscredsx509': ['crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c',
-                                   tasn1, crypto],
+                                   tasn1, crypto, gnutls],
       'test-crypto-tlssession': ['crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c', 'crypto-tls-psk-helpers.c',
-                                 tasn1, crypto],
+                                 tasn1, crypto, gnutls],
       'test-io-channel-tls': ['io-channel-helpers.c', 'crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c',
-                              tasn1, io, crypto]}
+                              tasn1, io, crypto, gnutls]}
   endif
   if 'CONFIG_AUTH_PAM' in config_host
     tests += {'test-authz-pam': [authz]}
diff --git a/ui/meson.build b/ui/meson.build
index 013258a01c..e6655c94a6 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -29,7 +29,7 @@  vnc_ss.add(files(
   'vnc-ws.c',
   'vnc-jobs.c',
 ))
-vnc_ss.add(zlib, png, jpeg)
+vnc_ss.add(zlib, png, jpeg, gnutls)
 vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
 softmmu_ss.add_all(when: vnc, if_true: vnc_ss)
 softmmu_ss.add(when: vnc, if_false: files('vnc-stubs.c'))