GNOME Bugzilla – Bug 775950
Add Meson build support in grilo core
Last modified: 2017-02-14 10:29:41 UTC
Meson build is a fast and modern build tool, and GNOME is moving torwards using it. So let's add support in Grilo too.
Created attachment 341761 [details] [review] doc: put manpages in doc/man Put all manpage files together.
Created attachment 341762 [details] [review] core: include @filename@ in file-production Small fix for an include that is in the wrong place.
Created attachment 341763 [details] [review] build: add Meson build support
Created attachment 341767 [details] [review] build: add Meson build support
Review of attachment 341761 [details] [review]: Looks good
Review of attachment 341762 [details] [review]: sure
Review of attachment 341767 [details] [review]: Looks good otherwise. Fast as usual. Super great, really. ::: doc/grilo/meson.build @@ +22,3 @@ + 'quick-start.xml', + 'writing-apps.xml', + 'overview.xml', You are using a mix of tabs and spaces here. I would suggest to keep only spaces instead of spaces to keep consistency. ::: doc/man/meson.build @@ +10,3 @@ + 'grl-inspect-0.3.1', + 'grl-launch-0.3.1', +] Can we use the 'grilo-test-ui-@0@'.format(grl_majorminor) (for the others too) ::: src/grl-metadata-key.h @@ +128,3 @@ +/* END CORE KEYS */ + Shouldn't it be in a different patch? ::: tools/grilo-inspect/generate_core_keys.py @@ +34,3 @@ + +finput.close() +foutput.close() I would suggest to introduce this as a preparatory patch and perhaps changing the $(AWK) line in tools/grilo-inspect/Makefile.am to use this (or not, it is simple enough there but it does not make usage of /* BEGIN/END */) Not a requirement, just a suggestion :)
Review of attachment 341767 [details] [review]: ::: doc/grilo/meson.build @@ +22,3 @@ + 'quick-start.xml', + 'writing-apps.xml', + 'overview.xml', Sure. My fault. ::: doc/man/meson.build @@ +10,3 @@ + 'grl-inspect-0.3.1', + 'grl-launch-0.3.1', +] Uhm... That would return 'grilo-test-ui-0.3', not 'grilo-test-ui-0.3.1'. So we would need also to rename the manpages. And fix also the Makefile.am Is it fine for you? ::: src/grl-metadata-key.h @@ +128,3 @@ +/* END CORE KEYS */ + I kept in this patch because this is used by generate_core_keys.py, which is invoked in Meson when building grl-inspect. For the autotools version the own Makefile uses the old version, which doesn't requires this change. ::: tools/grilo-inspect/generate_core_keys.py @@ +34,3 @@ + +finput.close() +foutput.close() The reason why I'm not using this script in Makefile is because it would introduce a new dependency: python. This is not a problem with Meson, because python is required by Meson itself. Said that, do you mind if we leave this change for a future improvement?
Comment on attachment 341761 [details] [review] doc: put manpages in doc/man Attachment 341761 [details] pushed as 000303d - doc: put manpages in doc/man
Comment on attachment 341762 [details] [review] core: include @filename@ in file-production Attachment 341762 [details] pushed as 5940808 - core: include @filename@ in file-production
(In reply to Juan A. Suarez Romero from comment #8) > ::: doc/man/meson.build > @@ +10,3 @@ > + 'grl-inspect-0.3.1', > + 'grl-launch-0.3.1', > +] > > Uhm... That would return 'grilo-test-ui-0.3', not 'grilo-test-ui-0.3.1'. So > we would need also to rename the manpages. And fix also the Makefile.am > > Is it fine for you? I think it would make sense to not have to bump the meson.build every release so I was fine with an extra variable; But I don't think it is bad idea to bound the manual to major and minor values only too. > > ::: src/grl-metadata-key.h > @@ +128,3 @@ > > +/* END CORE KEYS */ > + > > I kept in this patch because this is used by generate_core_keys.py, which is > invoked in Meson when building grl-inspect. > > For the autotools version the own Makefile uses the old version, which > doesn't requires this change. > > ::: tools/grilo-inspect/generate_core_keys.py > @@ +34,3 @@ > + > +finput.close() > +foutput.close() > > The reason why I'm not using this script in Makefile is because it would > introduce a new dependency: python. Yeah but as far as I understood, it would be for build time only. > This is not a problem with Meson, because python is required by Meson itself. > > Said that, do you mind if we leave this change for a future improvement? Sure, not a problem at all. The awk line is quite simple to follow there too :)
(In reply to Victor Toso from comment #11) > (In reply to Juan A. Suarez Romero from comment #8) > > ::: doc/man/meson.build > > @@ +10,3 @@ > > + 'grl-inspect-0.3.1', > > + 'grl-launch-0.3.1', > > +] > > > > Uhm... That would return 'grilo-test-ui-0.3', not 'grilo-test-ui-0.3.1'. So > > we would need also to rename the manpages. And fix also the Makefile.am > > > > Is it fine for you? > > I think it would make sense to not have to bump the meson.build every > release so I was fine with an extra variable; But I don't think it is bad > idea to bound the manual to major and minor values only too. This is also an improvement that we can do later on, if needed.
Created attachment 345706 [details] [review] build: add Meson build support
Review of attachment 345706 [details] [review]: Other than that, looks great! ::: tools/grilo-test-ui/meson.build @@ +17,3 @@ + 'flickr-oauth.c', + 'flickr-oauth.h', + ] I think you also need to cdata.set('HAVE_OAUTH', true) otherwise it will be ignored due #ifdefs
Created attachment 345710 [details] [review] build: add Meson build support
Created attachment 345714 [details] [review] build: add Meson build support
Created attachment 345715 [details] [review] build: add Meson build support
Created attachment 345716 [details] [review] build: add Meson build support
Attachment 345716 [details] pushed as 4538c07 - build: add Meson build support