GNOME Bugzilla – Bug 792699
Various meson related improvements
Last modified: 2019-02-22 03:51:44 UTC
This bug includes various meson improvements.
Created attachment 367107 [details] [review] meson: Rename the package includedir The `includedir` used for the headers of the package itself has been usually named `pkgincludedir`. This patch creates a new variable to hold the name along the api version and uses this to create a new `pkgincludedir` variable which is going to be used instead of the `real includedir`. The subdir parameter of install headers has also been changed to use the path relative to the system header directory instead of the absolute path.
Created attachment 367108 [details] [review] meson: Use the pkgconfig module meson features a module called `pkgconfig` that can be used to generate pkgconfig `.pc` files. This patch takes advantage of this module to generate the pkgconfig `.pc` file.
Created attachment 367109 [details] [review] meson: Simplified introspection option check The build of the introspection data depends on the existence of the GObject library, the `g-ir-scanner` program, and the build not being a cross build. The check to the GObject library has been removed, because it is required for compiling libgit2. The gir variable has also been removed because there is no need to store the intermediate state. Finally, the `build_gir` variable has been renamed to remain consistent with other similar variables like `enable_ssh`.
Created attachment 367110 [details] [review] meson: Applied meson guidelines for SSH option Following the meson porting guidelines[0], the `ssh` option has been changed to be a boolean option. The options behaviour has also been slightly simplified by moving the configuration object to the libgit2-glib where the `ggit.h.in` actually resides. An auxiliary variable has also been set to avoid writing the file name twice which can be error prone. [0] https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting
Created attachment 367111 [details] [review] meson: Improve gtkdoc documentation generation The gtkdoc option has been changed to follow the meson porting guidelines[0]. A greater use of different variables is done to avoid any typo when rewriting the different strings that are needed for `gtkdoc` function parameters. When querying the glib dependency prefix, now the variable holding the previous dependency search is reused. The list of private headers has also been removed, and now the list present in meson build from the `libgit2-glib` directory is reused. Finally the links present in the generated documentation are fixed by using the `--html-dir` option. [0] https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting
Created attachment 367112 [details] [review] meson: Remove host system variable The variable holding information about the host system is not used at all. This patch removes this unused variable.
Created attachment 367113 [details] [review] meson: Improve compiler flags and debugging options meson provides two options related to debug options. The first one is the build type, which among others, can be `debug` build or `debugoptimized`. The second option which is called `b_ndebug` can be used to enable or disable assertions. The meson guidelines[0] recommends the use of options present in meson, so the combination of these two options has been used to replace the `debug` option. - `debug` build sets the `LIBGIT2_GLIB_ENABLE_DEBUG` macro, which is equivalent to `debug=yes`. - `debugoptimized` build sets the same macros as `debug`, but also set `G_DISABLE_CAST_CHECKS`, which is equivalent to `debug=minimum`. - If build is not a `debug*` build it sets `G_DISABLE_CAST_CHECKS` and `-G_DISABLE_CHECKS`. - If `b_ndebug` is true it sets `G_DISABLE_ASSERT`. The compiler flags are now only checked and used on debug builds. To ease the compiler flags checking the `get_supported_arguments` helper function is used, which implies bumping meson version to 0.43. [0] https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting
Created attachment 367114 [details] [review] meson: Use meson's naming convention The convention when naming dependencies is to use the '_dep' suffix, which helps detecting dependency variables. The variables have been renamed to follow this convention.
Created attachment 367115 [details] [review] meson: Reorganize included directories The `libgit2_glib_inc` variable holds two directories, the source root and also the `libgit2-glib` directory. However, this is not necessary, because a variable for the source root already exists, which is called `core_inc`, and the example programs which use the `libgit2_glib_inc` variable don't need the source root include. Now the `libgit2_glib_inc` variable only holds the `libgit2-glib` directory.
Created attachment 367116 [details] [review] meson: Remove unnecesary introspection options Since meson 0.43, the `generate_gir` function has a parameter called `header` which can be used as the name of the header to be included by the library. This patch uses this parameter to avoid using the `--include-header` separately. The `--identifier-prefix` has also been removed because the `identifier_prefix` from `generate_gir` is already used.
Created attachment 367117 [details] [review] meson: Use a variable for the namespace string The namespace string is used in multiple places in the meson's build files. This patch uses a variable which eases the use of this string through the build files.
Created attachment 367118 [details] [review] meson: Add support for VAPI generation autotools generates the VAPI files for the introspection files and is missing in meson. This patch adds support for VAPI generation in meson.
Created attachment 367121 [details] [review] meson: Add python support autotools features optional python support, which makes some checks regarding python and pygobject versions and compiles the installed python files. This patch adds the equivalent python support in meson.
Created attachment 367199 [details] [review] build: Remove autotools Although meson should be reviewed and fix any problems that may arise, I think it is on par in features with autotools. To avoid the burden of maintaining multiple build systems, this patch removes autotools support.
Review of attachment 367107 [details] [review]: Sure
Review of attachment 367108 [details] [review]: Fine for me
Review of attachment 367109 [details] [review]: OK
Review of attachment 367110 [details] [review]: OK
Review of attachment 367111 [details] [review]: OK
Review of attachment 367112 [details] [review]: Sure
Review of attachment 367113 [details] [review]: See the question inline ::: meson.build @@ +56,1 @@ + if cc.get_id() == 'msvc' Are you sure that we should not add all the flags for all the build types?
Review of attachment 367114 [details] [review]: Looks good
Review of attachment 367115 [details] [review]: Sure
Review of attachment 367116 [details] [review]: Sure
Review of attachment 367117 [details] [review]: Sure
Review of attachment 367118 [details] [review]: Sure
Review of attachment 367121 [details] [review]: OK
Review of attachment 367199 [details] [review]: Sure
(In reply to Ignacio Casal Quinteiro (nacho) from comment #21) > Review of attachment 367113 [details] [review] [review]: > > See the question inline > > ::: meson.build > @@ +56,1 @@ > + if cc.get_id() == 'msvc' > > Are you sure that we should not add all the flags for all the build types? My reasoning on this is that, by default anyone interested on looking and hacking the code would like a `debug` build. That's why `debugoptimized` is set as default option. In that case the warnings from `msvc_recommended_pragmas.h` will be added. Otherwise, `plain` and `release` build[0] options exists, useful for making just a release or packaging libgit2-glib. I consider that in those cases they actually might not be interested on the warning messages. I might be missing something, so please let me know what is your opinion/preference about this and I would change it accordingly. [0] http://mesonbuild.com/Running-Meson.html#configuring-the-source
https://git.gnome.org/browse/glib/tree/msvc_recommended_pragmas.h#n32 About the warnings ok, but we would me missing though defines...
Comment on attachment 367107 [details] [review] meson: Rename the package includedir Pushed as 245c656 - meson: Rename the package includedir
Comment on attachment 367108 [details] [review] meson: Use the pkgconfig module Pushed as b617886 - meson: Use the pkgconfig module
Comment on attachment 367109 [details] [review] meson: Simplified introspection option check Pushed as 966bd0b - meson: Simplified introspection option check
Comment on attachment 367110 [details] [review] meson: Applied meson guidelines for SSH option Pushed as 2075aea - meson: Applied meson guidelines for SSH option
Comment on attachment 367111 [details] [review] meson: Improve gtkdoc documentation generation Pushed as cad800a - meson: Improve gtkdoc documentation generation
Comment on attachment 367112 [details] [review] meson: Remove host system variable Pushed as f7be772 - meson: Remove host system variable
Created attachment 367200 [details] [review] meson: Improve compiler flags and debugging options (In reply to Ignacio Casal Quinteiro (nacho) from comment #30) > https://git.gnome.org/browse/glib/tree/msvc_recommended_pragmas.h#n32 > > About the warnings ok, but we would me missing though defines... Ok, you are right, I missed the defines. I've never build any gnome related packages on windows and I'm not fully aware of these kind of things. I have enabled all warning flags in both windows and non windows systems by default now (which is also done by other packages[0]). [0] https://github.com/ebassi/graphene/blob/master/meson.build#L54
Review of attachment 367200 [details] [review]: Fine for me now
Comment on attachment 367200 [details] [review] meson: Improve compiler flags and debugging options Pushed as 5691ce6 - meson: Improve compiler flags and debugging options
Comment on attachment 367114 [details] [review] meson: Use meson's naming convention Pushed as 88dd8f6 - meson: Use meson's naming convention
Comment on attachment 367115 [details] [review] meson: Reorganize included directories Pushed as 21de993 - meson: Reorganize included directories
Comment on attachment 367116 [details] [review] meson: Remove unnecesary introspection options Pushed as 8d8995c - meson: Remove unnecessary introspection options
Comment on attachment 367117 [details] [review] meson: Use a variable for the namespace string Pushed as db7b5e8 - meson: Use a variable for the namespace string
Comment on attachment 367118 [details] [review] meson: Add support for VAPI generation Pushed as 0b098d4 - meson: Add support for VAPI generation
Comment on attachment 367121 [details] [review] meson: Add python support Pushed as 5f2a419 - meson: Add python support
Everything is in place except `Remove autotools` (attachment 367199 [details] [review]) patch. I'd like to give some time to see if there are any issues in the transition, but it's true that having autotools in the tree may discourage from using/trying meson. My proposal: there are still some steps to be made for the full transition from autotools to meson. Until recently, some changes were needed for jhbuild and gnome-continuous. However, with the arrival of BuildStream even more changes might be needed. Taking this in consideration, I would maintain autotools until everything all the changes are made and they are successful. This would keep the transition gap short, but it would leave some time to check and fix the issues. What do you think?
The first issue has arised and I was forced to do an emergency commit[0] because it broke gnome-continuous[1]. This issue happened becaue an unexpected behaviour change has recently been introduced a in meson[2], and also because gnome-continuous is using a developer version of meson. I have actually post a message[3] about what happened. [0] https://git.gnome.org/browse/libgit2-glib/commit/?id=b8e9bfae149eafe9840385ec63b055d4d0aaf92d [1] http://build.gnome.org/continuous/buildmaster/builds/2018/01/22/8/build/log-libgit2-glib.txt [2] https://github.com/mesonbuild/meson/pull/1896 [3] https://github.com/mesonbuild/meson/pull/1896#issuecomment-359383136
I have already made some steps that are needed to remove autotools: - gnome-continuous: Everything is already in place (which also produced the error in comment 47)- - jhbuild: The changes are already in place[0]. I have tried it and it works perfectly. - buildstream: Recently the `gnome-build-meta` repo has been presented[1] and I have pushed a MR to move libgit2-glib to meson[2]. I have done a full build in my computer and it also works perfectly. I guess that autotools could be removed once the buildstream's MR is merged. [0] https://git.gnome.org/browse/jhbuild/commit/?id=bcd21df34834c53418ba285563b0d1fc706d82fa [1] https://mail.gnome.org/archives/desktop-devel-list/2018-January/msg00025.html [2] https://gitlab.gnome.org/GNOME/gnome-build-meta/merge_requests/2
Review of attachment 367108 [details] [review]: ::: libgit2-glib/meson.build @@ +194,3 @@ + description: 'libgit2-glib, a a glib wrapper library around the libgit2 git access library.', + filebase: libgit2_glib_api_name, + subdirs: join_paths(libgit2_glib_api_name, meson.project_name()), 'subdirs' should be 'libgit2_glib_api_name'. We cannot add 'libgit2-glib' directory to the include path because users are expected to use '#include <libgit2-glib/ggit.h>', which already includes the directory name 'libgit2-glib'. It breaks gnome-builder because the test program cannot be compiled. Meson encountered an error in file meson.build, line 222, column 4: Error encountered: libgit2 was not compiled with -DTHREADSAFE:BOOL=ON Native dependency libgit2-glib-1.0 found: YES 0.26.2 Running compile: Working directory: /tmp/tmp7c9mekz4 Command line: clang -L/home/lantw44/gnome/devinstall/lib -L/usr/local/lib -I/home/lantw44/gnome/devinstall/include/libgit2-glib-1.0/libgit2-glib -I/home/lantw44/gnome/devinstall/include/glib-2.0 -I/home/lantw44/gnome/devinstall/lib/glib-2.0/include /tmp/tmpng1g4uuj/testfile.c -DGIT_SSH=1 -pthread -Wl,--start-group -lgit2-glib-1.0 -lgit2 -lgio-2.0 -lgobject-2.0 -lglib-2.0 -lintl -Wl,--end-group -march=corei7 -B/home/lantw44/.local/bin -pipe -g3 -O0 -O0 -std=gnu11 -D_FILE_OFFSET_BITS=64 -o /tmp/tmp7c9mekz4/output.exe Code: #include <libgit2-glib/ggit.h> int main(int argc, const char *argv[]) { ggit_init (); return ((ggit_get_features() & GGIT_FEATURE_THREADS) != 0) ? 0 : 1; } Compiler stdout: Compiler stderr: /tmp/tmp7c9mekz4/testfile.c:2:14: fatal error: 'libgit2-glib/ggit.h' file not found #include <libgit2-glib/ggit.h> ^~~~~~~~~~~~~~~~~~~~~ 1 error generated. Could not compile test file /tmp/tmp7c9mekz4/testfile.c: 1
Created attachment 367659 [details] [review] meson: Fix compiler flags in pkgconfig file (In reply to Ting-Wei Lan from comment #49) > Review of attachment 367108 [details] [review] [review]: > 'subdirs' should be 'libgit2_glib_api_name'. We cannot add 'libgit2-glib' > directory to the include path because users are expected to use '#include > <libgit2-glib/ggit.h>', which already includes the directory name > 'libgit2-glib'. Thanks for noticing the issue. This patch should fix it. Could you please try it? > It breaks gnome-builder because the test program cannot be compiled. > > Could not compile test file /tmp/tmp7c9mekz4/testfile.c: 1 I haven't been able to find this file in the source tree. Is it something related to `gnome-builder`?
Review of attachment 367659 [details] [review]: I tested this patch and can confirm that it fixes build error on GNOME Builder
(In reply to Corentin Noël from comment #51) > Review of attachment 367659 [details] [review] [review]: > > I tested this patch and can confirm that it fixes build error on GNOME > Builder attachment 367659 [details] [review] pushed as 7f5a50c - meson: Fix compiler flags in pkgconfig file Thank you for testing it! :)
The only missing patch is the one that removes autotools. However, due to the cycle freeze probably it shouldn't be applied now. Ignacio has the last word here.
Dunno I would just go ahead and remove it, but not sure how many distributors we would piss off...
I have been asking in the release team channel in the IRC about this. We could remove autotools and release .90, so distributions can ship and test it, so we can fix any issue that might arise before .0 is released. I will apply it and remove autotools in a couple of hours, once I get back home.
attachment 367199 [details] [review] pushed as 572d590 - build: Remove autotools
Created attachment 367962 [details] [review] meson: Ignore SSH headers when disabled I have been thoroughly testing the meson port and I have spotted an issue with documentation generation when SSH is disabled. This patch fixes this issue.
Created attachment 368080 [details] [review] meson: Fix GIR generation The generated GIR information does not include proper header location and package name. This patch fixes both parameters.
Created attachment 368091 [details] [review] meson: Fix GIR generation An update to the last patch, that also includes enums information in the generated GIR file.
Created attachment 368092 [details] [review] meson: Add or ignore SSH headers depending on the SSH option The SSH headers were not explicitly included when building because the compiler founds them. However, they are necessary for proper GIR information generation.
Review of attachment 368091 [details] [review]: Looks good
Review of attachment 368092 [details] [review]: See the comment otherwise go ahead and push it. ::: libgit2-glib/meson.build @@ +206,3 @@ libgit2_glib = shared_library('git2-glib-@0@'.format(libgit2_glib_api_version), include_directories: core_inc, + sources: sources + enum_types, should we have headers here as well?
(In reply to Ignacio Casal Quinteiro (nacho) from comment #62) > Review of attachment 368092 [details] [review] [review]: > > See the comment otherwise go ahead and push it. > > ::: libgit2-glib/meson.build > @@ +206,3 @@ > libgit2_glib = > shared_library('git2-glib-@0@'.format(libgit2_glib_api_version), > include_directories: core_inc, > + sources: sources + enum_types, > > should we have headers here as well? afaik, they're actually not necessary for building the library, because the compiler will find them by inspecting the sources. On the other hand, `enum_types` holds both source and header, but in this case, this is because they are generated sources and they need to be added here so ninja is able to create proper dependencies. although it's a bit unrelaeted, if you have some spare time, could you please take a look to this issue?: https://bugzilla.gnome.org/show_bug.cgi?id=787198
Comment on attachment 368091 [details] [review] meson: Fix GIR generation Pushed as d055813 - meson: Fix GIR generation
Anything else missing here?
I think that we are mostly done. Only one patch is missing[0], that was already reviewed[1], and there was an answer to a comment[2]. If you're okay with it, it can be pushed and this bug closed. [0] https://bugzilla.gnome.org/show_bug.cgi?id=792699#c60 [1] https://bugzilla.gnome.org/show_bug.cgi?id=792699#c62 [2] https://bugzilla.gnome.org/show_bug.cgi?id=792699#c63
If you don't mind as would still rather see the headers as part of the sources. Apart from that feel free to push it when you change that.
Created attachment 368608 [details] [review] meson: Add or ignore SSH headers depending on the SSH option (In reply to Ignacio Casal Quinteiro (nacho) from comment #67) > If you don't mind as would still rather see the headers as part of the > sources. Apart from that feel free to push it when you change that. Fair enough. Headers can be removed later. Here goes the patch update and I'll push it now.
Comment on attachment 368608 [details] [review] meson: Add or ignore SSH headers depending on the SSH option Pushed as baf9dae - meson: Add or ignore SSH headers depending on the SSH option