| Message ID | 20210102125213.41279-1-r.bolshakov@yadro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | meson: Propagate gnutls dependency | expand |
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
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
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
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
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
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
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
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
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',
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
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
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
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
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()],
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',
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', >
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
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 >
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
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
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 > >
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
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 --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'))
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(-)