GNOME Bugzilla – Bug 783562
Meson build instructions for libmediaart
Last modified: 2017-08-23 14:25:19 UTC
This is a rewrite of the libmediaart build instructions using Meson. We would gain faster builds and simpler build instructions by switching to Meson.
Created attachment 353416 [details] [review] Remove --enable-nemo and always use Qt5 'minimal' backend The 'minimal' Qt platform abstraction seems to be available on most platforms, it's not Nemo-specific. So we can use it unconditionally. This is the only thing affected by --enable-nemo so we can remove that configure flag.
Created attachment 353417 [details] [review] Meson build instructions for libmediaart These are hopefully complete already. I have compared an Autotools-built and a Meson-built install of libmediaart and found only the following differences: * storage.h is installed by Meson but not by Autotools * libmediaart-2.0.la isn't generated by Meson * External references in the gtk-doc documentation are relative with Meson and absolute with Autotools
Created attachment 353419 [details] [review] Meson build instructions for libmediaart These are hopefully complete already. I have compared an Autotools-built and a Meson-built install of libmediaart and found only the following differences: * storage.h is installed by Meson but not by Autotools * libmediaart-2.0.la isn't generated by Meson * External references in the gtk-doc documentation are relative with Meson and absolute with Autotools
Review of attachment 353416 [details] [review]: Makes sense, I've got a question about it though ::: libmediaart/extractqt.cpp @@ +63,1 @@ setenv("QT_QPA_PLATFORM", "minimal", 1); What happens if 'minimal' is not supported? you get a warning? plain out failure? I can live with the former, but I'm cautious about the latter.
Review of attachment 353419 [details] [review]: Looks mostly good! A couple of comments about the storage bits though. I also wonder if it makes sense to switch entirely to meson for libmediaart, it certainly seems more of a no-brainer than it is for Tracker :) ::: libmediaart/meson.build @@ +4,3 @@ + 'extractgeneric.h', + 'mediaart.h', + 'storage.h' Do we need installing storage.h? It's not even properly namespaced :/. I wouldn't say it's part of the public API. And btw... is there anything preventing these symbols from being exported? nm seems to confirm they are :/. In autotools we have -export-symbols-regex '^media_art_.*' in LDFLAGS, I wonder if we should just go for explicit extern instead.
> + 'storage.h' > > Do we need installing storage.h? It's not even properly namespaced :/. I > wouldn't say it's part of the public API. I would even go as far as: Do we need the whole storage? It was used for .mediaart local, currently it does not serve any purpose at all other then locking up the calling process when the GVFS setup is wocky (waiting for VolumeMonitors to appear) > > And btw... is there anything preventing these symbols from being exported? > nm seems to confirm they are :/. In autotools we have > > -export-symbols-regex '^media_art_.*' > > in LDFLAGS, I wonder if we should just go for explicit extern instead. You can use a simple versioning script, put everything in the unversioned "namespace" like { media_art_*; };
(In reply to Carlos Garnacho from comment #4) > Review of attachment 353416 [details] [review] [review]: > > Makes sense, I've got a question about it though > > ::: libmediaart/extractqt.cpp > @@ +63,1 @@ > setenv("QT_QPA_PLATFORM", "minimal", 1); > > What happens if 'minimal' is not supported? you get a warning? plain out > failure? I can live with the former, but I'm cautious about the latter. It would fail to start if the platform plugin isn't found, see: https://code.qt.io/cgit/qt/qtbase.git/tree/src/gui/kernel/qguiapplication.cpp#n1119 Looking at the build instructions though, the 'minimal' platform plugin is built everywhere except on Android: https://code.qt.io/cgit/qt/qtbase.git/tree/src/plugins/platforms/platforms.pro However, I think that setting this environment variable has no effect at all because we're using QCoreApplication not QGuiApplication so there's no platform abstraction loaded in the first place. Setting QT_QPA_PLATFORM to garbage doesn't seem to break anything. So I guess we can remove that code anyway.
(In reply to Carlos Garnacho from comment #5) > Review of attachment 353419 [details] [review] [review]: > > Looks mostly good! A couple of comments about the storage bits though. I > also wonder if it makes sense to switch entirely to meson for libmediaart, > it certainly seems more of a no-brainer than it is for Tracker :) Up to you, but I think it would make sense and would be a good canary for switching over Tracker :-) > ::: libmediaart/meson.build > @@ +4,3 @@ > + 'extractgeneric.h', > + 'mediaart.h', > + 'storage.h' > > Do we need installing storage.h? It's not even properly namespaced :/. I > wouldn't say it's part of the public API. We don't need to do that, it just made the build instructions easier. I'll remove it again. > And btw... is there anything preventing these symbols from being exported? > nm seems to confirm they are :/. In autotools we have > > -export-symbols-regex '^media_art_.*' > > in LDFLAGS, I wonder if we should just go for explicit extern instead. Good spot. The Meson equivalent of this is to write a linker script it seems (see http://mesonbuild.com/FAQ.html#how-do-i-do-the-equivalent-of-libtools-exportsymbol-and-exportregex). Or we could use __attribute__((visibility)) but if anyone is building with MSVC that would thwart them.
(In reply to Sam Thursfield from comment #8) > (In reply to Carlos Garnacho from comment #5) > > Review of attachment 353419 [details] [review] [review] [review]: > > > > Looks mostly good! A couple of comments about the storage bits though. I > > also wonder if it makes sense to switch entirely to meson for libmediaart, > > it certainly seems more of a no-brainer than it is for Tracker :) > > Up to you, but I think it would make sense and would be a good canary for > switching over Tracker :-) > > > ::: libmediaart/meson.build > > @@ +4,3 @@ > > + 'extractgeneric.h', > > + 'mediaart.h', > > + 'storage.h' > > > > Do we need installing storage.h? It's not even properly namespaced :/. I > > wouldn't say it's part of the public API. > > We don't need to do that, it just made the build instructions easier. I'll > remove it again. This is actually an issue in the Autotools builds as well, because the rules to run introspection go over storage.h as well, and they complain a lot. :-) > > And btw... is there anything preventing these symbols from being exported? > > nm seems to confirm they are :/. In autotools we have > > > > -export-symbols-regex '^media_art_.*' > > > > in LDFLAGS, I wonder if we should just go for explicit extern instead. > > Good spot. The Meson equivalent of this is to write a linker script it seems > (see > http://mesonbuild.com/FAQ.html#how-do-i-do-the-equivalent-of-libtools- > exportsymbol-and-exportregex). Or we could use __attribute__((visibility)) > but if anyone is building with MSVC that would thwart them. MSVC can be handled with a similar annotation; an example is the stanza in Pango: https://git.gnome.org//browse/pango/tree/meson.build#n128 Additionally, by default MSVC builds with all symbols marked as private, so you either mark them with a compiler annotation or you use a symbols file.
Created attachment 356113 [details] [review] Remove --enable-nemo This flag was just enabling a codepath that sets QT_QPA_PLATFORM=minimal in the environment before constructing a QCoreApplication instance. This code path has no effect anyway: QT_QPA_PLATFORM only has an effect if you are using QGuiApplication.
Created attachment 356114 [details] [review] Use compiler symbol visiblity features to hide internal functions Previously we relied on libtool's -export-symbols-regex feature, but we are hoping to drop GNU Autotools and GNU Libtool in soon in favour of Meson. Meson doesn't have an equivalent feature, instead the advice is to control symbol visibility at compile time. The approach taken in this patch is based on Pango's build system. Pango tells the compiler to hide symbols by default (if possible), and then defines a _PANGO_EXTERN variable at compile time which marks a single symbol as public. In Pango's case there is then further machinary to hide symbols based on deprecation policies but I have not copied that here, instead I used _LIBMEDIAART_EXTERN directly. If a compiler doesn't support hiding symbols then the library we build makes all symbols available, which is exactly what would happen before on platforms where the libtool didn't have an implementation for -export-symbols-regex. See also: http://mesonbuild.com/FAQ.html#how-do-i-do-the-equivalent-of-libtools-exportsymbol-and-exportregex https://git.gnome.org/browse/pango/ https://git.gnome.org/browse/pango/tree/pango/pango-version-macros.h https://gcc.gnu.org/wiki/Visibility https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#Code-Gen-Options (-fvisibility)
Created attachment 356115 [details] [review] Meson build instructions for libmediaart These are hopefully complete already. I have compared an Autotools-built and a Meson-built install of libmediaart and found only the following differences: * libmediaart-2.0.la isn't generated by Meson * External references in the gtk-doc documentation are relative with Meson and absolute with Autotools * Some changes in generated .vapi file and .pc file
Thanks for the reviews and advice; new version fixes a bunch of stuff including: - Removed QT_QPA_PLATFORM stuff which has no effect - Implemented symbol visibility properly - Stopped installing storage.h
Comment on attachment 356113 [details] [review] Remove --enable-nemo Sure. Less cruft :).
Comment on attachment 356114 [details] [review] Use compiler symbol visiblity features to hide internal functions Makes sense to do this.
Comment on attachment 356115 [details] [review] Meson build instructions for libmediaart It looks and works just alright AFAICS :).
Review of attachment 356115 [details] [review]: The .la file is generated by libtool, and Meson does not use libtool. You don't need empty meson.build files inside sub-directories: you can use `subdir('docs/reference/libmediaart')` directly. ::: config.h.meson.in @@ +1,1 @@ +/* config.h.in. Generated from configure.ac by autoheader. */ This comment is not really true any more :-) You also don't necessarily need a template config.h file with Meson: if you use configure_file() without an input, Meson will generate one for you using all the keys in the configuration_data object. See, for instance: https://github.com/ebassi/graphene/commit/389e8ede2786b8dab937b9a65f4ca11b7ab2b8ab ::: docs/reference/libmediaart/meson.build @@ +1,3 @@ +version_xml = configure_file(input: 'version.xml.in', + output: 'version.xml', + configuration: conf) See above, re: no input file. Additionally, you may want a specific XML entities file, e.g.: https://github.com/ebassi/graphene/blob/master/doc/xml/meson.build @@ +5,3 @@ +gnome.gtkdoc('libmediaart', + src_dir: 'libmediaart', + main_sgml: 'libmediaart-docs.sgml', You should take the chance to rename this to `libmediaart-docs.xml` and use `main_xml`. :-) ::: libmediaart/meson.build @@ +44,3 @@ + extra_args: [ + '--c-include=libmediaart/mediaart.h', + '--cflags-begin'] + libmediaart_cflags + ['--cflags-end', No, this is not necessary; g-ir-scanner understands -I, -D, and -U perfectly fine. ::: meson.build @@ +1,1 @@ +project('libmediaart', 'c', 'cpp', version: '1.9.1') It's a good idea to add things like `default_options`, e.g.: https://github.com/ebassi/graphene/blob/master/meson.build#L1 @@ +3,3 @@ +gnome = import('gnome') + +gnome = import('gnome') Duplicate import. @@ +32,3 @@ + image_library_name = 'gdk-pixbuf-2.0' + endif +elif get_option('image_library') == 'icu' There is no 'icu' option in the meson_options.txt file. @@ +33,3 @@ + endif +elif get_option('image_library') == 'icu' + error('gdk-pixbuf backend explicitly requested, but gdk-pixbuf library was not found') This error() doesn't really make sense, in a check for `icu` as the image library. @@ +95,3 @@ + +subdir('libmediaart') +subdir('docs') You can use `subdir('docs/reference/libmediaart')`.
This work was finished off by Carlos with the review comments addressed and is now merged to master. Thanks Emmanuele for the help!