[Spice-devel] [PATCH spice-server 1/2] Add support for building with meson
Eduardo Lima (Etrunko)
etrunko at redhat.com
Wed Apr 11 18:45:06 UTC 2018
On 11/04/18 12:30, Frediano Ziglio wrote:
>>
>> 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.
Yes, I know -Wall does not enable everything, but I thought -Wextra
would do the trick. Do you know whether it is enough or we need to
enable even more warnings manually?
>
>> 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.
Most of those checks that I mentioned are in spice-common, while server
indeed does very few checks of system headers.
>
>>>> +#
>>>> +# 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?
>
The defines are indeed inherited, but the options must be explicitly
defined in both subproject and superproject, even though the checks are
done in spice-common. Otherwise meson will error out:
WARNING: Unknown command line options: "alignment-checks"
This will become a hard error in a future Meson release.
>> 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
>
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com
More information about the Spice-devel
mailing list