Created attachment 369004 [details] [review]
Port to meson build system

Work in progress, early comments are welcome.

Thinks that need to be sorted out:
* gtk-doc building is failing: figure out how to deal with xml files that need pre-processing or avoid having to pre-process them.
* There is probably more.

Review of attachment 369004 [details] [review]:

Thanks for the initiative!

For meson I have only one non-standard requirement:

Add this comment at the top of the root meson.build file:

# Convention:
# - Local variables in lower_case.
# - Global variables in UPPER_CASE.
# See: https://github.com/mesonbuild/meson/issues/2607

And follow the convention, it'll make the code easier to understand.

Please modify the Autotools files in separate commits.

Review of attachment 369004 [details] [review]:

::: meson.build
@@ +164,3 @@
+
+subdir('docs')
+subdir('po')

There is no po/meson.build.

Created attachment 369182 [details] [review]
Make sure build targets have unique names

Created attachment 369189 [details] [review]
Avoid substitutions in gtk-doc's xml content files

Created attachment 369190 [details] [review]
Port to meson build system

Still work in progress, should be mostly find now though.

Review of attachment 369182 [details] [review]:

I don't understand why it is needed. Is it because there is the same binary name in another directory? If meson cannot handle that…

Review of attachment 369189 [details] [review]:

The commit message should explain why the change is done.

Maybe Splinter is buggy, version-api.xml.in and version.xml.in are empty. gtk-doc has a single file for gtkdocentities, so it's normally possible to have also only one file with additional variables. And is version.xml actually used?

::: docs/reference/gspell-docs.xml.in
@@ +15,3 @@
+      This document is for the gspell &apiversion; library, version &version;.
+      The latest versions can be found online at
+      <ulink role="online-location" url="https://developer.gnome.org/gspell/">https://developer.gnome.org/gspell/</ulink>.

Why do you sneak in such change? This should be added with a separate commit.

And I prefer not adding that information. With meson developer.gnome.org is not updated… (yet).

Review of attachment 369190 [details] [review]:

You didn't follow my convention to distinguish global variables from local variables.

I'm not sure Meson is the right choice. Bazel looks like better, see:
https://en.wikipedia.org/wiki/Bazel_(software)

(Meson looks like a toy compared to Bazel).

(In reply to Sébastien Wilmet from comment #7)
> I don't understand why it is needed. Is it because there is the same binary
> name in another directory? If meson cannot handle that…

The meson build system currently impose target names to be unique across a project. This is a design decision rather than a limitation [1]. An effort pushing toward the removal of this limitation seems to exist, though [2].
This patch is proposing a source rename so that executable and source names match, but this is not mandatory. But, so far, renaming one of the two executable is. My feeling is that renaming executable from gspell's tests folder is not a big deal, but of course this opinion may not be shared, and I may miss something here...

(In reply to Sébastien Wilmet from comment #8)
> Maybe Splinter is buggy, version-api.xml.in and version.xml.in are empty.
> gtk-doc has a single file for gtkdocentities, so it's normally possible to
> have also only one file with additional variables. And is version.xml
> actually used?

Totally right, will fix that.

> ::: docs/reference/gspell-docs.xml.in
> @@ +15,3 @@
> +      This document is for the gspell &apiversion; library, version
> &version;.
> +      The latest versions can be found online at
> +      <ulink role="online-location"
> url="https://developer.gnome.org/gspell/">https://developer.gnome.org/gspell/
> </ulink>.
> 
> Why do you sneak in such change? This should be added with a separate commit.
> 
> And I prefer not adding that information. With meson developer.gnome.org is
> not updated… (yet).

Ok.

(In reply to Sébastien Wilmet from comment #9)
> You didn't follow my convention to distinguish global variables from local
> variables.

I have to admit that I'm a bit confused here. There is no such thing as local variables in meson. Do you expect variables used in multiple files to be upper case? Because I'm worried that this will lead to future case-only one-line changes here and there for variables that where not used outside of their declaration file but eventually became used elsewhere.

(In reply to Sébastien Wilmet from comment #10)
> I'm not sure Meson is the right choice. Bazel looks like better, see:
> https://en.wikipedia.org/wiki/Bazel_(software)
> 
> (Meson looks like a toy compared to Bazel).

Well, meson is the build system that has been chosen by GNOME, but of course that decision is yours. If porting to meson is not wanted anymore, please close that bug.

[1] https://github.com/mesonbuild/meson/pull/1867#issuecomment-304692989
[2] https://github.com/mesonbuild/meson/issues/2861

Review of attachment 369182 [details] [review]:

.

::: tests/Makefile.am
@@ +14,3 @@
 
+TEST_PROGS += test-text-entry
+test_text_entry_SOURCES = test-text-entry.c

The widget is named GtkEntry, not GtkTextEntry. So I prefer to keep the source file as is, and in meson have target names test-entry-unit-test (in testsuite/) and test-entry-gui-test (in tests/).

I think Bazel has a more solid foundation than Meson, but for GNOME a lot of macros/extensions are missing with Bazel and I don't want to develop all those extensions (for GSettings, GResources, GIR, GTK-Doc, enum types, etc etc). So in the end, yes the plan is to still port the build system to Meson, even if I don't like everything with Meson.

For local variables vs global variables, see comment 0 at:
https://github.com/mesonbuild/meson/issues/2607

"It would be better to be able to differentiate local variables (those used in only one meson.build file) from global variables (those used in several meson.build files)."

In Meson all variables are global and in the same scope, but what I mean by "local" variable is the intent, i.e. that variable is intended to be used only in the same meson.build file. When a variable is intended to be used in other meson.build files, some care must be taken, and it's a useful fact to know when reading and trying to understand the code.

In GTK+:
$ wc -l **/meson.build | tail -1
  5572 total

So it's the equivalent of 5000 lines of code in a single file, with all the variables in the same scope. Put it this way, this is clearly a programming horror story… But for a small project like gspell, this shouldn't be a major problem if you follow my convention ;-)

I will re-open the same feature request on GitLab.