| Message ID | 20210428144813.417170-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofsd: Meson build fixes | expand |
On Wed, 28 Apr 2021 at 15:55, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: (cc'ing Paolo for a meson.build question below...) > When not explicitly select a sysemu target and building virtiofsd, > the seccomp/cap-ng libraries are not resolved, leading to this error: > > $ configure --target-list=i386-linux-user --disable-tools --enable-virtiofsd > tools/meson.build:12:6: ERROR: Problem encountered: virtiofsd requires libcap-ng-devel and seccomp-devel > > Fix by checking the seccomp/cap-ng libraries if virtiofsd is built. > > Reported-by: Mahmoud Mandour <ma.mandourr@gmail.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > meson.build | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/meson.build b/meson.build > index c6f4b0cf5e8..b466b418fed 100644 > --- a/meson.build > +++ b/meson.build > @@ -393,14 +393,14 @@ > endif > > seccomp = not_found > -if not get_option('seccomp').auto() or have_system or have_tools > +if not get_option('seccomp').auto() or have_system or have_tools or not get_option('virtiofsd').auto() > seccomp = dependency('libseccomp', version: '>=2.3.0', > required: get_option('seccomp'), > method: 'pkg-config', kwargs: static_kwargs) > endif > > libcap_ng = not_found > -if not get_option('cap_ng').auto() or have_system or have_tools > +if not get_option('cap_ng').auto() or have_system or have_tools or not get_option('virtiofsd').auto() > libcap_ng = cc.find_library('cap-ng', has_headers: ['cap-ng.h'], > required: get_option('cap_ng'), > kwargs: static_kwargs) Now we have "virtiofsd requires cap-ng and seccomp" recorded in three places in different meson.build files: * here, if this patch goes in * in tools/meson.build, in its logic for setting have_virtiofsd (I generously do not count the "decide which error message to print" logic in that file as a separate item in this list...) * in tools/virtiofsd/meson.build, where the executable('virtiofsd', ...) lists them in its dependencies: setting Is there some way to avoid this duplication? thanks -- PMM
On 4/28/21 5:06 PM, Peter Maydell wrote: > On Wed, 28 Apr 2021 at 15:55, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > (cc'ing Paolo for a meson.build question below...) > >> When not explicitly select a sysemu target and building virtiofsd, >> the seccomp/cap-ng libraries are not resolved, leading to this error: >> >> $ configure --target-list=i386-linux-user --disable-tools --enable-virtiofsd >> tools/meson.build:12:6: ERROR: Problem encountered: virtiofsd requires libcap-ng-devel and seccomp-devel >> >> Fix by checking the seccomp/cap-ng libraries if virtiofsd is built. >> >> Reported-by: Mahmoud Mandour <ma.mandourr@gmail.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> meson.build | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/meson.build b/meson.build >> index c6f4b0cf5e8..b466b418fed 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -393,14 +393,14 @@ >> endif >> >> seccomp = not_found >> -if not get_option('seccomp').auto() or have_system or have_tools >> +if not get_option('seccomp').auto() or have_system or have_tools or not get_option('virtiofsd').auto() >> seccomp = dependency('libseccomp', version: '>=2.3.0', >> required: get_option('seccomp'), >> method: 'pkg-config', kwargs: static_kwargs) >> endif >> >> libcap_ng = not_found >> -if not get_option('cap_ng').auto() or have_system or have_tools >> +if not get_option('cap_ng').auto() or have_system or have_tools or not get_option('virtiofsd').auto() >> libcap_ng = cc.find_library('cap-ng', has_headers: ['cap-ng.h'], >> required: get_option('cap_ng'), >> kwargs: static_kwargs) > > Now we have "virtiofsd requires cap-ng and seccomp" recorded in three > places in different meson.build files: > * here, if this patch goes in > * in tools/meson.build, in its logic for setting have_virtiofsd > (I generously do not count the "decide which error message to print" > logic in that file as a separate item in this list...) > * in tools/virtiofsd/meson.build, where the executable('virtiofsd', ...) > lists them in its dependencies: setting > > Is there some way to avoid this duplication? I noticed that too, I think the problem is we never considered the case of a standalone binary buildable without sysemu / tools, such virtiofsd. TIL virtiofsd is not a tool, so tools/meson.build needs rework, in particular to remove the 'have_tools' but maybe to move it out of tools/: have_virtiofsd = (targetos == 'linux' and have_tools and seccomp.found() and libcap_ng.found() and 'CONFIG_VHOST_USER' in config_host) If virtiofsd requires Linux, shouldn't we check it directly in ./configure, not allowing --enable-virtiofsd on non-linux hosts? Maybe it is there for cross-compilation... Is it supported to cross-build it on non-Linux hosts? It could be clearer to use: want_virtiofsd = get_option('virtiofsd').auto() BTW to test I used: $ configure --target-list=i386-linux-user --disable-tools --enable-virtiofsd $ ninja tools/virtiofsd/virtiofsd
On 4/28/21 7:48 AM, Philippe Mathieu-Daudé wrote: > seccomp = not_found > -if not get_option('seccomp').auto() or have_system or have_tools > +if not get_option('seccomp').auto() or have_system or have_tools or not get_option('virtiofsd').auto() > seccomp = dependency('libseccomp', version: '>=2.3.0', > required: get_option('seccomp'), > method: 'pkg-config', kwargs: static_kwargs) This construct is wrong, both before and after, as I read it. not get_option(foo).auto() is true for both enabled and disabled. If disabled, why are we examining the dependency? If auto, if we have all of the dependencies we want to enable the feature -- if we don't probe for the dependency, how can we enable it? This error seems to be offset by the OR have_* tests, for which the logic also seems off. I think the test should have been if (have_system or have_tools) and (not get_option('seccomp').disabled() or not get_option('virtiofsd').disabled()) Then we need to combine the required: argument, probably like required: get_option('seccomp').enabled() or get_option('virtiofsd').enabled() r~
On 4/28/21 6:34 PM, Richard Henderson wrote: > On 4/28/21 7:48 AM, Philippe Mathieu-Daudé wrote: >> seccomp = not_found >> -if not get_option('seccomp').auto() or have_system or have_tools >> +if not get_option('seccomp').auto() or have_system or have_tools or >> not get_option('virtiofsd').auto() >> seccomp = dependency('libseccomp', version: '>=2.3.0', >> required: get_option('seccomp'), >> method: 'pkg-config', kwargs: static_kwargs) > > This construct is wrong, both before and after, as I read it. > > not get_option(foo).auto() is true for both enabled and disabled. If > disabled, why are we examining the dependency? If auto, if we have all > of the dependencies we want to enable the feature -- if we don't probe > for the dependency, how can we enable it? > > This error seems to be offset by the OR have_* tests, for which the > logic also seems off. > > I think the test should have been > > if (have_system or have_tools) and Yes but virtiofsd is not a tool... It is a standalone binary. Maybe have_system is the culprit here: have_system = have_system or target.endswith('-softmmu') We should somewhere add: have_system = have_system or something('virtiofsd') However I wonder if we aren't going to build many objects that are irrelevant for virtiofsd. > (not get_option('seccomp').disabled() or > not get_option('virtiofsd').disabled()) > > Then we need to combine the required: argument, probably like > > required: get_option('seccomp').enabled() or > get_option('virtiofsd').enabled() > > > r~ >
On Wed, 28 Apr 2021 at 19:02, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 4/28/21 6:34 PM, Richard Henderson wrote: > > I think the test should have been > > > > if (have_system or have_tools) and > > Yes but virtiofsd is not a tool... It is a standalone binary. This is not a distinction that our build/configure system currently makes. We have three categories: * user-mode emulator binaries * system-mode emulator binaries * tools where essentially a "tool" is "any binary we build that isn't a QEMU binary proper". If virtiofsd is genuinely not related to QEMU at all, what is it doing in our source tree ? thanks -- PMM
On 4/28/21 8:00 PM, Philippe Mathieu-Daudé wrote: > On 4/28/21 6:34 PM, Richard Henderson wrote: >> On 4/28/21 7:48 AM, Philippe Mathieu-Daudé wrote: >>> seccomp = not_found >>> -if not get_option('seccomp').auto() or have_system or have_tools >>> +if not get_option('seccomp').auto() or have_system or have_tools or >>> not get_option('virtiofsd').auto() >>> seccomp = dependency('libseccomp', version: '>=2.3.0', >>> required: get_option('seccomp'), >>> method: 'pkg-config', kwargs: static_kwargs) >> >> This construct is wrong, both before and after, as I read it. >> >> not get_option(foo).auto() is true for both enabled and disabled. If >> disabled, why are we examining the dependency? If auto, if we have all >> of the dependencies we want to enable the feature -- if we don't probe >> for the dependency, how can we enable it? >> >> This error seems to be offset by the OR have_* tests, for which the >> logic also seems off. >> >> I think the test should have been >> >> if (have_system or have_tools) and > > Yes but virtiofsd is not a tool... It is a standalone binary. > Maybe have_system is the culprit here: > > have_system = have_system or target.endswith('-softmmu') > > We should somewhere add: > > have_system = have_system or something('virtiofsd') So this hunk does fix the issue ...: -- >8 -- --- a/meson.build +++ b/meson.build @@ -52,4 +52,5 @@ endforeach have_tools = 'CONFIG_TOOLS' in config_host +# virtiofsd depends on sysemu +have_system = have_system or not get_option('virtiofsd').disabled() have_block = have_system or have_tools --- > However I wonder if we aren't going to build many objects > that are irrelevant for virtiofsd. Based on top of https://www.mail-archive.com/qemu-devel@nongnu.org/msg799069.html to remove libsoftfloat, 216 objects are required to build virtiofsd: [216/216] Linking target tools/virtiofsd/virtiofsd This one-line fix seems good enough (to keep virtiofsd as a 'tool').
diff --git a/meson.build b/meson.build index c6f4b0cf5e8..b466b418fed 100644 --- a/meson.build +++ b/meson.build @@ -393,14 +393,14 @@ endif seccomp = not_found -if not get_option('seccomp').auto() or have_system or have_tools +if not get_option('seccomp').auto() or have_system or have_tools or not get_option('virtiofsd').auto() seccomp = dependency('libseccomp', version: '>=2.3.0', required: get_option('seccomp'), method: 'pkg-config', kwargs: static_kwargs) endif libcap_ng = not_found -if not get_option('cap_ng').auto() or have_system or have_tools +if not get_option('cap_ng').auto() or have_system or have_tools or not get_option('virtiofsd').auto() libcap_ng = cc.find_library('cap-ng', has_headers: ['cap-ng.h'], required: get_option('cap_ng'), kwargs: static_kwargs)
When not explicitly select a sysemu target and building virtiofsd, the seccomp/cap-ng libraries are not resolved, leading to this error: $ configure --target-list=i386-linux-user --disable-tools --enable-virtiofsd tools/meson.build:12:6: ERROR: Problem encountered: virtiofsd requires libcap-ng-devel and seccomp-devel Fix by checking the seccomp/cap-ng libraries if virtiofsd is built. Reported-by: Mahmoud Mandour <ma.mandourr@gmail.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- meson.build | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)