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

Wed Apr 11 15:05:02 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.

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.

>> +#
>> +# 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.

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.

-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com