GNOME Bugzilla – Bug 786639
Port glib-networking to meson build system
Last modified: 2017-10-21 15:54:08 UTC
Created attachment 358178 [details] [review] Port to meson build system Port to meson build system of glib-networking package. Although it is working fine, when building and installing, there is an issue with testing. TLS protocol tests are expecting test data files (in files directory) to be in te same folder, which works with autotools because build and source code files are mixed. However, meson is intended to build files on a separate folder, which is nice, but test data files aren't available until installed on the intended directory. Regarding 'gio modules' dir, autotools is trying to install them on the directory specified on the 'gio-2.0.pc' file, which is pointing to the system 'libdir' directory. I have implemented the same behaviour in the meson build system, although other packages (gvfs, dconf) use the 'libdir' directory (usually under 'prefix'), which allows installing the package by any user avoiding system 'libdir'. This latter behaviour is also implemented, though it's commented.
Created attachment 358179 [details] [review] Remove autotools Some applications are removing autotools to avoid the burden of maintaining two build systems. This patch removes autotools in case it is considered.
I haven't been able to do it until now because I've had a limited Internet connection, but I've now created a testing branch to ease its testing: https://git.gnome.org/browse/glib-networking/log/?h=wip/inigomartinez/meson
Review of attachment 358178 [details] [review]: Let's get this merged. I have a bunch of style nits, but that's pretty much all. ::: glib-networking.map @@ +1,2 @@ +{ +global: What is this file for? ::: meson.build @@ +2,3 @@ + 'glib-networking', 'c', + version: '2.53.90', + license: 'GPL2+', Heh, no thank you, it's LGPL2.1+ @@ +5,3 @@ + default_options: [ + 'buildtype=debugoptimized', + 'warning_level=1' Isn't debugoptimized and warning_level=1 already the defaults? If not, what are the defaults? @@ +10,3 @@ +) + +networking_version = meson.project_version() Probably easiest to just use meson.project_version() wherever this is needed. @@ +12,3 @@ +networking_version = meson.project_version() + +networking_prefix = get_option('prefix') Please drop the networking_ prefix from all these variables, it's just confusing. I thought e.g. that networking_libexecdir was pkglibexecdir until I looked at the definition here and discovered it was not. @@ +18,3 @@ +networking_localedir = join_paths(networking_prefix, get_option('localedir')) + +networking_installed_test_metadir = join_paths(networking_datadir, 'installed-tests', meson.project_name()) Ditto. @@ +34,3 @@ + ['PACKAGE_TARNAME', meson.project_name()], + ['PACKAGE_URL', ''], + ['PACKAGE_VERSION', networking_version], What are all these PACKAGE_ things for? Pretty sure almost none of these are actually needed in config.h. I think you can remove them all (or at least almost all of them). @@ +37,3 @@ + ['VERSION', networking_version], + # i18n + ['GETTEXT_PACKAGE', meson.project_name()] Except for this one, which looks important. ;) @@ +40,3 @@ +] + +foreach define: set_defines Then this won't need to be a foreach. @@ +45,3 @@ + +# headers +check_headers = [ None of these checks are used anywhere in glib-networking, so these should all be removed too. @@ +63,3 @@ + +# functions +check_functions = [ And none of these checks are needed either. @@ +72,3 @@ +if host_machine.system().contains('darwin') + check_functions += [ + ['HAVE_CFLOCALECOPYCURRENT', 'CFLocaleCopyCurrent'], Ditto. @@ +92,3 @@ +if get_option('buildtype').contains('debug') + test_flags = [ + '-Werror=declaration-after-statement', I'd remove all this -Werror stuff. Now that we're switching to meson, we can use a nice standard meson warning_level. If there's a bunch of warnings with 2, then 1 is good enough for now. @@ +104,3 @@ + foreach flag: test_flags + if cc.has_argument(flag) + common_flags += [flag] So then this won't be needed. @@ +109,3 @@ +endif + +add_project_arguments(common_flags, language: 'c') Nor this. @@ +114,3 @@ + +test_ldflags = [ + '-Wl,--version-script,' + symbol_map, Why is this needed? @@ +121,3 @@ +if host_machine.system().contains('linux') + foreach ldflag: test_ldflags + if cc.has_argument(ldflag) This seems pretty extreme... if the linker doesn't support these flags, the user is going to have a hard time getting any software to work. We don't need to be so careful about portability checks that will never fail in practice. So I would just remove the checks. @@ +133,3 @@ +gio_module_dir = gio_dep.get_pkgconfig_variable('giomoduledir') +assert(gio_module_dir != '', 'GIO_MODULE_DIR is missing from gio-2.0.pc') +# FIXME: what about using ${libdir}/gio/modules? Nope... as unfortunate as it is to potentially install files outside of the prefix, there's no point is second-guessing what pkg-config tells us. You can remove the FIXME and the commented line below. @@ +139,3 @@ +have_libproxy = false + +enable_libproxy = get_option('with-libproxy') It's sad that we don't have a consistent standard for whether to use hyphens or underscores, but I've settled on underscores for my projects. I also prefer to remove the with/enable prefixes. So let's rename this option to simply 'libproxy_backend'. @@ +149,3 @@ +have_gnome_proxy = false + +enable_gnome_proxy = get_option('with-gnome-proxy') And this one to 'gnome_proxy_backend' @@ +159,3 @@ +tls_support = [] + +enable_gnutls = get_option('with-gnutls') It's hard to believe anyone would want to turn this off, but I guess we'd better not mess with the available options in this patch. Let's call this one 'tls_support' But if you want to make this required and remove the build option, that's fine with me too. @@ +184,3 @@ + res = run_command(test_cmd, '-e', cert_file) + + assert(res.returncode() != 0, msg + 'could not find. Use -Dwith-ca-certificates=path to set, or -Dwith-ca-certificates=no to disable') Let's make this mandatory: 'Use -Dca_certificates_path=PATH to set, or -Dtls_support=no to disable' And make sure that works, of course. @@ +196,3 @@ + if res.returncode() != 0 + ca_certificates = 'no' + message(msg + ca_certificates + ' does not exist') "no does not exist"? That's not quite right.... @@ +204,3 @@ + have_pkcs11 = false + + enable_pkcs11 = get_option('with-pkcs11') 'pkcs11_support' Also fine by me if you want to make this required. @@ +235,3 @@ +subdir('po') + +enable_installed_tests = get_option('enable-installed-tests') 'installed_tests' ::: meson_options.txt @@ +4,3 @@ +option('with-ca-certificates', type: 'string', value: '', description: 'path to system Certificate Authority list') +option('with-pkcs11', type: 'combo', choices: ['yes', 'no', 'auto'], value: 'auto', description: 'support for pkcs11') +option('enable-installed-tests', type: 'boolean', value: false, description: 'enable installed unit tests') So all of these options should be renamed. :) ::: proxy/gnome/meson.build @@ +21,3 @@ +) + +test_programs += [['gnome', deps]] How does this work, considering there was never any 'gnome' test program built? ::: tls/tests/meson.build @@ +58,3 @@ + + # FIXME: it fails to locate data files because they are copied only on install time and not in compile time + # ./tap-driver.sh --test-name connection --log-file connection.log --trs-file connection.trs --color-tests yes --enable-hard-errors yes --expect-failure no -- ./tap-test _build/tls/tests/connection Isn't tap obsoleted by meson's own test runner functionality? Probably we should not be using it anymore, right?
Comment on attachment 358179 [details] [review] Remove autotools Can't wait to push this. ;)
Created attachment 361825 [details] [review] Port to meson build system Here goes an update following your comments. Thank you for taking your time to review the patch! Some comments below. (In reply to Michael Catanzaro from comment #3) > Review of attachment 358178 [details] [review] [review]: > > Let's get this merged. > > I have a bunch of style nits, but that's pretty much all. > > ::: glib-networking.map > @@ +1,2 @@ > +{ > +global: > > What is this file for? It's a linker-script[0] file. It hides all the symbols except `g_io_module_(load|unload|query)` in the shared modules. > @@ +5,3 @@ > + default_options: [ > + 'buildtype=debugoptimized', > + 'warning_level=1' > > Isn't debugoptimized and warning_level=1 already the defaults? If not, what > are the defaults? The default `builtype` is `debug`[1], however autotools use '-g -O2' by default, which is what `debugoptimized` applies. In the other hand, the default `warning_level` is 1[2], so I have removed it. > @@ +109,3 @@ > +endif > + > +add_project_arguments(common_flags, language: 'c') > > Nor this. This is necessary for some name predefinitions: `G_LOG_DOMAIN`, `G_DISABLE_DEPRECATED`, ... > > @@ +114,3 @@ > + > +test_ldflags = [ > + '-Wl,--version-script,' + symbol_map, > > Why is this needed? It's the `ld` linker flag used to specify the symbol map. > @@ +184,3 @@ > + res = run_command(test_cmd, '-e', cert_file) > + > + assert(res.returncode() != 0, msg + 'could not find. Use > -Dwith-ca-certificates=path to set, or -Dwith-ca-certificates=no to disable') > > Let's make this mandatory: 'Use -Dca_certificates_path=PATH to set, or > -Dtls_support=no to disable' > > And make sure that works, of course. > > @@ +196,3 @@ > + if res.returncode() != 0 > + ca_certificates = 'no' > + message(msg + ca_certificates + ' does not exist') > > "no does not exist"? That's not quite right.... Fixed, and as it's mandatory now, I have also changed the message, which is Now similar to the previous one. However, if the message is shown also in the summary I wonder if it should be just removed, and show only an error message in case of error. > ::: proxy/gnome/meson.build > @@ +21,3 @@ > +) > + > +test_programs += [['gnome', deps]] > > How does this work, considering there was never any 'gnome' test program > built? The built command is at `proxy/tests/meson.build`, which iterates over `test_programs` array. I considered this the proper place to append this test as this build file is where its dependencies are defined. I changed slightly how `meson.build` files are processed inside `proxy` directory, so I hope the scope of the `test_programs` array is more clear now. Finally, you do not have to run it manually, it runs as an unit test with `ninja builddir test`. > ::: tls/tests/meson.build > @@ +58,3 @@ > + > + # FIXME: it fails to locate data files because they are copied only on > install time and not in compile time > + # ./tap-driver.sh --test-name connection --log-file connection.log > --trs-file connection.trs --color-tests yes --enable-hard-errors yes > --expect-failure no -- ./tap-test _build/tls/tests/connection > > Isn't tap obsoleted by meson's own test runner functionality? Probably we > should not be using it anymore, right? I've not been involved in GNOME's development, and I haven't used `tap` before, so I don't know exactly what's the status of `tap`. Anyway, I tried to use these tests once they are built, without installing them, but they need some modifications because they were designed to be run in the source dir. I also asked for advice in meson's forum[3], and as I though, tests must be modified. I agree that it would be nice to move to `unit tests`, taking advantage of its support on meson, and particularly with the recent move of projects to GitLab, which also provides a way to do CI. [0] https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_mono/ld.html [1] https://github.com/mesonbuild/meson/blob/master/mesonbuild/coredata.py#L334 [2] https://github.com/mesonbuild/meson/blob/master/mesonbuild/coredata.py#L360 [3] https://groups.google.com/forum/#!topic/mesonbuild/0F56N2zpPwA
Created attachment 361826 [details] [review] Remove autotools A follow up update, because these patches has been updated after rebasing master with latest changes. (In reply to Michael Catanzaro from comment #4) > Comment on attachment 358179 [details] [review] [review] > Remove autotools > > Can't wait to push this. ;) Remember that if it's finally merged there are steps before/after to be done. One of the steps is related to `gnome-continuous`[1]. Until recently a `configure` script was necessary, however some changes are in progress[2], so ebassi should be contacted in this regard. The other step is related to `jhbuild` :) [0] https://mail.gnome.org/archives/desktop-devel-list/2017-April/msg00080.html [1] https://bugzilla.gnome.org/show_bug.cgi?id=789146#c3
Created attachment 361827 [details] [review] Make PKCS#11 support required As you suggested here goes a patch that makes `PKCS#11` support required. Maybe the message at summary should also be removed?
Created attachment 361828 [details] [review] Make TLS support required As you also suggested, here goes a patch that makes `TLS` support required. Also in this case, should the message at summary be removed?
(In reply to Iñigo Martínez from comment #5) > The default `builtype` is `debug`[1], however autotools use '-g -O2' by > default, which is what `debugoptimized` applies. In the other hand, the > default `warning_level` is 1[2], so I have removed it. I'm pretty sure autotools does not use either -g or -O2 by default. That's what Linux distros want, but distros have their own %meson macros and whatnot to handle this for them. For development, you really don't want to use -O2 as it's just going to make debugging harder. So I would omit this too, and stick to meson's defaults. > > @@ +114,3 @@ > > + > > +test_ldflags = [ > > + '-Wl,--version-script,' + symbol_map, > > > > Why is this needed? > > It's the `ld` linker flag used to specify the symbol map. I mean, how did this work in the Autotools build? We didn't need a .map file there. Anyway, the answer is in glib-networking.mk: module_flags = -export_dynamic -avoid-version -module -no-undefined -export-symbols-regex '^g_io_module_(load|unload|query)' I think using -export-symbols-regex would be slightly nicer than adding a new file. > I agree that it would be nice to move to `unit tests`, taking advantage of > its support on meson, and particularly with the recent move of projects to > GitLab, which also provides a way to do CI. I can look into this later. It shouldn't block this work.
Comment on attachment 361825 [details] [review] Port to meson build system Accepted, noting the comments above. But let's wait until Javier has finished this week's GNOME release, so we don't cause him problems by messing with the jhbuild module type.
I'll ask Dan about the other two patches. Not sure why anybody would ever build glib-networking without TLS support, or if PKCS#11 might ever be unavailable nowadays.
Created attachment 361837 [details] [review] Port to meson build system (In reply to Michael Catanzaro from comment #10) > Comment on attachment 361825 [details] [review] [review] > Port to meson build system > > Accepted, noting the comments above. But let's wait until Javier has > finished this week's GNOME release, so we don't cause him problems by > messing with the jhbuild module type. Let's improve it then. > For development, you really don't want to use -O2 as it's just going to make > debugging harder. So I would omit this too, and stick to meson's defaults. I have removed the `buildtype`, so it will use meson defaults now. > I mean, how did this work in the Autotools build? We didn't need a .map file > there. Anyway, the answer is in glib-networking.mk: > > module_flags = -export_dynamic -avoid-version -module -no-undefined > -export-symbols-regex '^g_io_module_(load|unload|query)' `export-symbols-regex` it's a libtool option, which is not supported by meson which uses `ld` directly. The `linker-script` is the proposed solution[0]. > I'll ask Dan about the other two patches. Not sure why anybody would ever > build glib-networking without TLS support, or if PKCS#11 might ever be > unavailable nowadays. Please, if Dan agrees, also consider my questions regarding the appearance of these options in the summary. Finally, I have updated the wip-branch[1] so it would be nice to test it until Javier finishes. [0] http://mesonbuild.com/FAQ.html#how-do-i-do-the-equivalent-of-libtools-exportsymbol-and-exportregex [1] https://git.gnome.org/browse/glib-networking/log/?h=wip/inigomartinez/meson
Created attachment 361839 [details] [review] Port to meson build system I've seen that you have been working on the 789171 - Regression on tests issue, so I've rebased again the meson patch, which also adds the missing files to the build file. However, I'm sorry to say that the issue is not solved yet. I've included a new comment which I hope helps.
Created attachment 361840 [details] [review] Remove autotools A follow up update.
Review of attachment 361839 [details] [review]: Looks generally good. ::: meson.build @@ +36,3 @@ +module_ldflags = [ + '-Wl,--no-undefined', + '-Wl,--version-script,' + symbol_map Meson will add `-Wl,--no-undefined` where needed for shared libraries, depending on the compiler and platform. Since you're using `shared_module()`, you really don't want `no-undefined`, as the symbols will be resolved when the module is dlopen'ed. Additionally, you want to check that the compiler being used understands `-Wl,--version-script` before adding it to the command line. I mean, we're going to compile this module with GCC or a GCC-compatible compiler, very likely, but the linker is another matter. @@ +77,3 @@ + msg = 'location of system Certificate Authority list: ' + ca_certificates_path = get_option('ca_certificates_path').strip() + test_cmd = find_program('test') Yuck. It's not really kosher to add shell script inside a meson.build. I'd replace this whole block with a Python script that took a list of files to check as the input, and returned the path of the certificates file found as the output. Then you'd be able to do: ``` if enable_tls_support cert_locations = [ get_option('ca_certificates_path'), '/etc/pki/tls/certs/ca-bunderl.crt', '/etc/ssl/certs/ca-certificates.crt', '/etc/ssl/ca-bundle.pem', ] check_certs = find_program('check_certs.py') res = run_program(check_certs, cert_locations) assert(res.returncode() != 0) ca_certificates_path = res.stdout().strip() if ca_certificates_path = '' error('Could not find any TLS certificate; use -Dca_certificates_path=PATH or -Dtls_support=no') endif endif ``` @@ +139,3 @@ +subdir('po') + +enable_installed_tests = get_option('installed-tests') You probably want `installed_tests`, to match the other options. @@ +143,3 @@ + +if have_libproxy or have_gnome_proxy + test_programs = [] This should probably be define unconditionally, in case we want to add tests in the future. ::: proxy/tests/meson.build @@ +27,3 @@ + ) + + test(program[0], exe) The tests use the GLib test API, which means you need to add an environment: ``` env: [ 'G_TEST_SRCDIR=@0@'.format(meson.current_source_dir()), 'G_TEST_BUILDDIR=@0@'.format(meson.current_build_dir()), ] ``` Otherwise GTest won't be able to find files. ::: tls/tests/meson.build @@ +60,3 @@ + # ./tap-driver.sh --test-name connection --log-file connection.log --trs-file connection.trs --color-tests yes --enable-hard-errors yes --expect-failure no -- ./tap-test _build/tls/tests/connection + # ./tap-test /tmp/gn/libexec/installed-tests/glib-networking/file-database + #test(program[0], exe, args: ['-k', '--tap']) Same as above, you also need an `env` with `G_TEST_SRCDIR` and `G_TEST_BUILDDIR` defined.
(In reply to Michael Catanzaro from comment #9) > > > @@ +114,3 @@ > > > + > > > +test_ldflags = [ > > > + '-Wl,--version-script,' + symbol_map, > > > > > > Why is this needed? > > > > It's the `ld` linker flag used to specify the symbol map. > > I mean, how did this work in the Autotools build? We didn't need a .map file > there. Because we're using libtool. > Anyway, the answer is in glib-networking.mk: > > module_flags = -export_dynamic -avoid-version -module -no-undefined > -export-symbols-regex '^g_io_module_(load|unload|query)' > > I think using -export-symbols-regex would be slightly nicer than adding a > new file. Meson doesn't use libtool, and those are libtool options. The appropriate replacement is a map file. (In reply to Michael Catanzaro from comment #3) > @@ +92,3 @@ > +if get_option('buildtype').contains('debug') > + test_flags = [ > + '-Werror=declaration-after-statement', > > I'd remove all this -Werror stuff. Now that we're switching to meson, we can > use a nice standard meson warning_level. If there's a bunch of warnings with > 2, then 1 is good enough for now. No, this is wrong. The error flags are there for a guaranteed set of errors that will trip a build failure. Meson's warning_level=2 only enables `-Wall` and a couple of other warnings; if the terrible history of AX_COMPILER_FLAGS has taught us anything is that projects *need* to define their own minimum set of supported compiler warnings that will result in a build failure. One size fits all approaches will inevitably lead people and machines building projects to deal with unresponsive maintainers, and compilers changing the set of warning between releases. > ::: tls/tests/meson.build > @@ +58,3 @@ > + > + # FIXME: it fails to locate data files because they are copied only on > install time and not in compile time > + # ./tap-driver.sh --test-name connection --log-file connection.log > --trs-file connection.trs --color-tests yes --enable-hard-errors yes > --expect-failure no -- ./tap-test _build/tls/tests/connection > > Isn't tap obsoleted by meson's own test runner functionality? Probably we > should not be using it anymore, right? It's not entirely necessary, but it doesn't hurt. There are test harnesses that can pick up the logs. TAP is the Test Anything Protocol; ideally, Meson's own test runner should support the protocol. There's a slight issue, here, with GLib having a broken TAP implementation that will just abort() on assertion failures, breaking the expected results; this should not really matter, though. Meson will, at some point, support the GLib/GNOME testing conventions natively, including TAP and installed tests.
Thanks for the feedback. Two more comments: (In reply to Emmanuele Bassi (:ebassi) from comment #15) > @@ +77,3 @@ > + msg = 'location of system Certificate Authority list: ' > + ca_certificates_path = get_option('ca_certificates_path').strip() > + test_cmd = find_program('test') > > Yuck. It's not really kosher to add shell script inside a meson.build. I actually prefer Inigo's approach to this. It's not shell script, it's just calling an external command to check if some files exist. Seems nicer to do it here than to add a new script, IMO. (In reply to Emmanuele Bassi (:ebassi) from comment #16) > No, this is wrong. > > The error flags are there for a guaranteed set of errors that will trip a > build failure. I really don't intend to keep a custom set of warning/Werror flags in each module I maintain, so please still do remove this list. My habit is to fix or suppress all warnings as soon as I notice them, anyway.
(In reply to Iñigo Martínez from comment #7) > Created attachment 361827 [details] [review] [review] > Make PKCS#11 support required > > As you suggested here goes a patch that makes `PKCS#11` support required. > Maybe the message at summary should also be removed? Yeah, of course the message summaries should be removed, and the conditions should be removed from the code as well. Don't worry about it, though; I'll do it myself in a separate bug if that's what we decide to do. I shouldn't have distracted you with that unrelated task. Thanks for working on it, though. :)
Created attachment 361904 [details] [review] Port to meson build system Thank you both for your comments. I've almost applied all the changes. I've also removed some definitions which weren't used at all. Only `HAVE_PKCS11` is used, but if finally `PKCS#11` is going to be built by default, this should also be removed. (In reply to Emmanuele Bassi (:ebassi) from comment #15) > Review of attachment 361839 [details] [review] [review]: > ::: meson.build > @@ +36,3 @@ > +module_ldflags = [ > + '-Wl,--no-undefined', > + '-Wl,--version-script,' + symbol_map > > Meson will add `-Wl,--no-undefined` where needed for shared libraries, > depending on the compiler and platform. Since you're using > `shared_module()`, you really don't want `no-undefined`, as the symbols will > be resolved when the module is dlopen'ed. > > Additionally, you want to check that the compiler being used understands > `-Wl,--version-script` before adding it to the command line. I mean, we're > going to compile this module with GCC or a GCC-compatible compiler, very > likely, but the linker is another matter. Although I know that meson adds `-Wl,--no-undefined`, I added the flag to ensure that it was going to be applied, as it can be disabled by setting the `b_undef` to false. However, following your advice I have removed it and restored the compiler's flag check. > > @@ +77,3 @@ > + msg = 'location of system Certificate Authority list: ' > + ca_certificates_path = get_option('ca_certificates_path').strip() > + test_cmd = find_program('test') > > Yuck. It's not really kosher to add shell script inside a meson.build. > > I'd replace this whole block with a Python script that took a list of files > to check as the input, and returned the path of the certificates file found > as the output. The approach looks clean to me, so I've implemented the `rather small` script and use it. > @@ +139,3 @@ > +subdir('po') > + > +enable_installed_tests = get_option('installed-tests') > > You probably want `installed_tests`, to match the other options. I don't get exactly this comment. > @@ +143,3 @@ > + > +if have_libproxy or have_gnome_proxy > + test_programs = [] > > This should probably be define unconditionally, in case we want to add tests > in the future. This array is only used for tests in the `proxy` directory. I have declared it outside to avoid making a second check inside tests directory. If it definitely is causing more confusion, I can change it. > ::: tls/tests/meson.build > @@ +60,3 @@ > + # ./tap-driver.sh --test-name connection --log-file connection.log > --trs-file connection.trs --color-tests yes --enable-hard-errors yes > --expect-failure no -- ./tap-test _build/tls/tests/connection > + # ./tap-test /tmp/gn/libexec/installed-tests/glib-networking/file-database > + #test(program[0], exe, args: ['-k', '--tap']) > > Same as above, you also need an `env` with `G_TEST_SRCDIR` and > `G_TEST_BUILDDIR` defined. I wasn't aware of those environment variables. Those programs are working now as unit tests inside the build directory! Thank you very much! I've also ended up removing the `-k` and `--tap` arguments, as I'm not sure if they are actually used. (In reply to Michael Catanzaro from comment #17) > I actually prefer Inigo's approach to this. It's not shell script, it's just > calling an external command to check if some files exist. Seems nicer to do > it here than to add a new script, IMO. I've read this now after creating the new python script. It's cleaner than it was before, and I also like more this approach. It's up to you to decide what's the approach to be implemented. > I really don't intend to keep a custom set of warning/Werror flags in each > module I maintain, so please still do remove this list. My habit is to fix > or suppress all warnings as soon as I notice them, anyway. I have removed the warnings. If you want them to be back, they could be added later.
(In reply to Michael Catanzaro from comment #18) > (In reply to Iñigo Martínez from comment #7) > > Created attachment 361827 [details] [review] [review] [review] > > Make PKCS#11 support required > > > > As you suggested here goes a patch that makes `PKCS#11` support required. > > Maybe the message at summary should also be removed? > > Yeah, of course the message summaries should be removed, and the conditions > should be removed from the code as well. Don't worry about it, though; I'll > do it myself in a separate bug if that's what we decide to do. I shouldn't > have distracted you with that unrelated task. Thanks for working on it, > though. :) I've got some updates to those patches, so don't hesitate to ask me for the files if you need them :)
> > @@ +139,3 @@ > > +subdir('po') > > + > > +enable_installed_tests = get_option('installed-tests') > > > > You probably want `installed_tests`, to match the other options. > > I don't get exactly this comment. Please rename it from 'installed-tests' to 'installed_tests' ;) > (In reply to Michael Catanzaro from comment #17) > > I actually prefer Inigo's approach to this. It's not shell script, it's just > > calling an external command to check if some files exist. Seems nicer to do > > it here than to add a new script, IMO. > > I've read this now after creating the new python script. It's cleaner than > it was before, and I also like more this approach. It's up to you to decide > what's the approach to be implemented. I'll defer to you on this, since you did all the work. > > I really don't intend to keep a custom set of warning/Werror flags in each > > module I maintain, so please still do remove this list. My habit is to fix > > or suppress all warnings as soon as I notice them, anyway. > > I have removed the warnings. If you want them to be back, they could be > added later. Yup, thanks!
Created attachment 361913 [details] [review] Port to meson build system (In reply to Michael Catanzaro from comment #21) > Please rename it from 'installed-tests' to 'installed_tests' ;) Awww... It was sooo evident :( It's fixed now.
Comment on attachment 361840 [details] [review] Remove autotools I'll commit these as soon as Javier is done with GNOME 3.27.1. Changing build systems during release week is a great way to annoy release team.
We also need to remember to remove the .gitignore files. And I need to not forget to update Continuous like I usually do.
I reworked the python script a little bit to move the list of files to check into the script. Also I got rid of all the evil automagic yes/no/auto build options. When porting to meson, we should almost always take the opportunity to change those to true/false booleans. auto just results in features being disabled accidentally, which is unnecessarily painful for people building the library, so it only makes sense in really exceptional situations (e.g. in WebKit we automatically decide between OpenGL or OpenGLES based on what is installed, but that doesn't affect the overall feature set). This is actually a really big problem, and there's no better time to fix it than when changing the entire build system.