[Spice-devel] [PATCH spice-server 1/2] Add support for building with meson

Wed Apr 11 15:30:51 UTC 2018

> 
> On 11/04/18 08:27, Frediano Ziglio wrote:
> >> +#
> >> +# global C defines
> >> +#
> >> +spice_server_global_cflags = [#'-std=c99', # fails compiling
> >> spice-common/bitops.h
> >> +                              '-fvisibility=hidden',
> >> +                              '-DSPICE_SERVER_INTERNAL',
> >> +                              '-DHAVE_CONFIG_H',
> >> +                              '-Wall',
> >> +                              '-Wextra',
> >> +                              #'-Werror',
> >> +                              '-Wno-sign-compare',
> >> +                              '-Wno-unused-parameter']
> >> +
> > 
> > about this, autoconf spend a lot of time looking at a lot of warning
> > options, how can be done in Meson?
> > 
> 
> It can be done with compiler.get_supported_arguments()
> 
> http://mesonbuild.com/Reference-manual.html#compiler-object
> 
> BUT, at least from what I understood by looking at the m4 file which
> does all those checks, it basically adds warning suppression, is it
> really interesting for us to shut up all those warnings?
> 
> In my point of view we should have fewer warning supressions, not more,
> as they can give hints about bugs which we do not know about yet. For
> instance, -Wno-sign-compare messages might be related to integer
> overflows, but I tried to build without -Wno-sign-compare, and there are
> way too many places to fix.
> 

No, there are also inclusions, -Wall, despite its name, does not enable
all warnings, some are enabled with that file.

> Also, it seems that file was copied out from some GL source code,
> another reason I wonder if this really belongs here.
> 
> 
> > On overall I think we should have a transition period where we
> > move from a system to the other trying to have the new one
> > feature equivalent. For instance rpm spec still uses autoconf stuff.
> > 
> 
> Sure, the rpm bits are missing. I think the best approach is to have a
> new meson specific spec file, and then on the removal patch rename it
> back to the original name.
> 
> >> +foreach arg : spice_server_global_cflags
> >> +  add_global_arguments(arg, language : 'c')
> >> +endforeach
> >> +
> >> +# other global vars
> >> +compiler = meson.get_compiler('c')
> >> +spice_server_config_data = configuration_data()
> >> +spice_server_config_data.set_quoted('VERSION', meson.project_version())
> >> +spice_protocol_min_version='0.12.14'
> >> +spice_server_include = [include_directories('.')]
> >> +spice_server_libs = []
> >> +spice_server_deps = []
> >> +spice_server_link_args = []
> >> +spice_server_requires = ''
> >> +
> >> +#
> >> +# Spice common subproject
> >> +#
> >> +spice_common_options =
> >> ['protocol-version=@0@'.format(spice_protocol_min_version),
> >> +                        'generate-code=server']
> >> +
> >> +spice_common = subproject('spice-common', default_options :
> >> spice_common_options)
> >> +spice_server_config_data.merge_from(spice_common.get_variable('spice_common_config_data'))
> >> +spice_server_include += spice_common.get_variable('spice_common_include')
> >> +
> >> +foreach dep : ['spice_common_deps', 'spice_common_server_dep']
> >> +  spice_server_deps += [spice_common.get_variable(dep)]
> >> +endforeach
> >> +
> >> +foreach lib : ['spice_common_lib', 'spice_common_server_lib']
> >> +  spice_server_libs += spice_common.get_variable(lib)
> >> +endforeach
> >> +
> >> +#
> >> +# check for system headers
> >> +#
> >> +headers = [['sys/time.h', 'HAVE_SYS_TIME_H'],
> >> +           ['execinfo.h', 'HAVE_EXECINFO_H'],
> >> +           ['linux/sockios.h', 'HAVE_LINUX_SOCKIOS_H'],
> >> +           ['pthread_np.h', 'HAVE_PTHREAD_NP_H']]
> >> +
> > 
> > seems quite manual having to specify both header name and
> > macro name, can that be automated like in configure.ac?
> > How much Python Meson is?
> >
> 
> Not much Python, basically the syntax is equivalent, but it is not
> turing complete, so you can not have functions and reuse in other
> places. The closest approach to that is using the foreach statement.
> 
> http://mesonbuild.com/Contributing.html#turing-completeness
> 
> We would be required to have a patch in meson itself to make things done
> automatically as done in AC_CHECK_HEADER.
> 
> And by the way, I noticed many of those macros in config.h are not even
> used by our code, would be a good time to revisit those as well.
> 

many are defined by some internal autoconf macros, I didn't find anything
we check from configure.ac directly which is not used in our code.
This does not mean that checking the existence of stdlib.h is useful
any more nowadays.

> >> +#
> >> +# Non-mandatory/optional dependencies
> >> +#
> >> +
> >> +# gstreamer
> >> +spice_server_has_gstreamer = false
> >> +spice_server_gst_version = get_option('gstreamer')
> >> +if spice_server_gst_version != 'no'
> >> +  gst_deps = ['gstreamer', 'gstreamer-base', 'gstreamer-app',
> >> 'gstreamer-video']
> >> +  foreach dep : gst_deps
> >> +    dep = '@0 at -@1@'.format(dep, spice_server_gst_version)
> >> +    spice_server_deps += dependency(dep)
> >> +  endforeach
> >> +  spice_server_deps += dependency('orc-0.4')
> >> +
> >> +  gst_def = 'HAVE_GSTREAMER'
> >> +  if spice_server_gst_version == '1.0'
> >> +    gst_def = 'HAVE_GSTREAMER_1_0'
> >> +  endif
> >> +
> >> +  spice_server_config_data.set(gst_def, '1')
> >> +  spice_server_has_gstreamer = true
> >> +endif
> >> +
> >> +# lz4
> >> +spice_server_has_lz4 = false
> >> +if get_option('lz4')
> >> +  lz4_dep = dependency('liblz4', required : false, version : '>= 129')
> >> +  if not lz4_dep.found()
> >> +    lz4_dep = dependency('liblz4', version : '>= 1.7.3')
> >> +  endif
> >> +
> >> +  spice_server_deps += lz4_dep
> >> +  spice_server_config_data.set('USE_LZ4', '1')
> >> +  spice_server_has_lz4 = true
> >> +endif
> >> +
> >> +# sasl
> >> +spice_server_has_sasl = false
> >> +if get_option('sasl')
> >> +  spice_server_deps += dependency('libsasl2')
> >
> > are these "dependency" using pkg-config? What about packages not
> > using pkg-config?
> >
> 
> Yes, they are implicitly using pkg-config. You can check for libraries
> with find_library(), programs with find_program() and header files with
> compiler.find_header(). Take a look at librt and libm cases for
> instance. The good thing is that the returned object is used the same
> way in both cases.
> 
> >> +
> >> +option('alignment-checks',
> >> +    type : 'boolean',
> >> +    value : false,
> >> +    description : 'Enable runtime checks for cast alignment
> >> (default=false)')
> >> +
> > 
> > why this is not in spice-common?
> > 
> 
> 'alignment-checks' option is defined here and in spice-common as well.
> As a subproject, spice-common will inherit it automatically from the
> super project, with the 'yield' keyword. This is one of the reasons we
> require meson version 0.45.
> 

Should not they be defines by spice-common and inherited by spice-server?
If I want to add an option to spice-common I need to add manually to
spice-server in order to be able to use it from spice-server?

> https://gitlab.com/etrunko/spice-common/blob/meson/meson_options.txt#L4
> 
> >> +if spice_server_has_smartcard == true
> >> +  spice_server_sources += ['smartcard.c',
> >> +                           'smartcard.h',
> >> +                           'smartcard-channel-client.c',
> >> +                           'smartcard-channel-client.h']
> > 
> > for coherence I'd use a syntax like
> > 
> >   spice_server_sources += [
> >     'smartcard.c',
> >     'smartcard.h',
> >     'smartcard-channel-client.c',
> >     'smartcard-channel-client.h',
> >   ]
> > 
> > (just space changes)
> > 
> > is possible to do something like
> > 
> >   spice_server_sources += '''
> >     smartcard.c
> >     smartcard.h
> >     smartcard-channel-client.c
> >     smartcard-channel-client.h
> >   '''.split()
> > 
> > ??
> >
> 
> I think it is possible yes, but I would go with the first option.
> 
> >> diff --git a/subprojects/spice-common.wrap b/subprojects/spice-common.wrap
> >> new file mode 100644
> >> index 00000000..6117c631
> >> --- /dev/null
> >> +++ b/subprojects/spice-common.wrap
> >> @@ -0,0 +1,4 @@
> >> +[wrap-git]
> >> +directory=spice-common
> >> +url=https://gitlab.com/etrunko/spice-common.git
> >> +revision=meson
> > 
> > is possible to have relative URL? So for instance you could
> > clone spice-server and spice-common and avoid having to change this file?
> > Something like
> > 
> > url=../spice-common.git
> > 
> 
> I don't think so, the subprojects folder is hardcoded and cannot be
> changed. Teuf proposed an auxiliary script that could be used by both
> autotools and meson but moving the git submodule under subprojects/.
> 
> https://paste.fedoraproject.org/paste/HceHsiV0cjLKg3S54X-gnQ
> 
> Regards, Eduardo.
> 

Frediano