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@-@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@xxxxxxxxxx _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel