GNOME Bugzilla – Bug 789118
Port to meson build system
Last modified: 2021-05-25 17:46:49 UTC
From the Meson official website [1]: 'Meson is an open source build system meant to be both extremely fast, and, even more importantly, as user friendly as possible'. Porting modules to Meson is a GNOME Goal [2]. GHex should probably follow that trend. [1] http://mesonbuild.com [2] https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting
Created attachment 361769 [details] [review] All-in-one migration path Port should be complete.
Hi, I'm used to port projects to Meson (and I did port GHex yesterday, didn't see you already did all the work :'( ), so I may have some remarks, even though that's a good port you did. # data/ You are using configure_file() to generate the pkgconfig file, why don't you use pkgconfig.generate() ? You won't need to use a configuration_data(), Meson handles everything. # help/ The 'linguas' argument of gnome.yelp() is deprecated. You should dump the list of languages to a LINGUAS file (just as in the po/ dir). Why 'symlink_media: false' ? symlinks would reduce the disk usage. That's a real question, i don't know if there's a real reason. # icons/ That doesn't change much, but you could just install_subdir() the whole 'hicolor' directory ;) # src/ You don't need to list headers inside sources. Actually, they will be ignored and that's just more work to maintain the list. libghex_dep = declare_dependency -> Is that really necessary ? Also, 'sources: libghex_headers' is *really* not what you want to do, I think. # overall * I personnally declared a variable containing 'gtkhex-3' so that the version number changes everything at the same time. You sometimes wrote "'gtkhex-@0@.0'.format(libghex_version_major)", but not everywhere. * Why check for all those headers/fns ? The project actually needs *none* of the 'HAVE_*' macros, so that's just noise in the meson code :/ * Same for 'ENABLE_NLS' * The rest of my remarks are basically codestyle, so i'll keep my personnal tastes for myself ;) BTW, just for reference if you want, my own port is here: https://github.com/Salamandar/ghex Hope you'll agree on some on my points ;) Salamandar
Totally makes sense what you say! How would this plan work for you both: First, we push Martin's initial port to master, and then Salamandar can do followup patches and fix up all the issues? :)
That's perfectly okay with me :D Obviously Martin should be okay with my future patches.
Comment on attachment 361769 [details] [review] All-in-one migration path OK, here we go -- pushed Martin's patch to master! Thanks, Martin! Let's keep this bug for the followup fixes as well, as all the relevant people are already CC'd here.
There is a minor issue in the current meson build files. The port to meson changes the subdirectory for installing headers from gtkhex-3 to gtkhex-3.0. It makes it inconsistent with Cflags specified in gtkhex-3.pc and causes nemiver to fail to build.
Created attachment 371223 [details] [review] build: Change header installation directory back to gtkhex-3 This is what autotools build used to use, and it makes it consistent with the .pc file.
Review of attachment 371223 [details] [review]: +1 from me, thanks.
Salamandar: did you have any further patches? You listed a bunch of issues above.
Comment on attachment 371223 [details] [review] build: Change header installation directory back to gtkhex-3 Attachment 371223 [details] pushed as 71a64b0 - build: Change header installation directory back to gtkhex-3
(In reply to Salamandar from comment #2) That was my first meson port, so yes, it was far from being perfect, sorry... Thanks for the review. > # data/ > You are using configure_file() to generate the pkgconfig file, why don't you > use pkgconfig.generate() ? You won't need to use a configuration_data(), > Meson handles everything. That was only keep because autotools were still in place and using it. We can probably remove it now, as autotools have been dropped. > # help/ > The 'linguas' argument of gnome.yelp() is deprecated. You should dump the > list of languages to a LINGUAS file (just as in the po/ dir). > Why 'symlink_media: false' ? symlinks would reduce the disk usage. > That's a real question, i don't know if there's a real reason. Yep, time to move to LINGUAS file. I can't remember why I decided not to use symlinks at the time. Can find any reasons now... > # icons/ > That doesn't change much, but you could just install_subdir() the whole > 'hicolor' directory ;) Sure, think I just wanted the name of the target directory to be a build system variable rather than the fs source directory name. > # src/ > You don't need to list headers inside sources. Actually, they will be > ignored and that's just more work to maintain the list. Agreed. > libghex_dep = declare_dependency -> Is that really necessary ? Also, > 'sources: libghex_headers' is *really* not what you want to do, I think. The dependency is only for convenience. You're right about the 'sources' though. > # overall > > * I personnally declared a variable containing 'gtkhex-3' so that the > version number changes everything at the same time. > You sometimes wrote "'gtkhex-@0@.0'.format(libghex_version_major)", but not > everywhere. Agreed, we need consistency here. > * Why check for all those headers/fns ? The project actually needs *none* of > the 'HAVE_*' macros, so that's just noise in the meson code :/ > * Same for 'ENABLE_NLS' Also agreed.
Created attachment 371958 [details] [review] Try to use consistent naming
Created attachment 371959 [details] [review] Clean up the build process
Created attachment 371960 [details] [review] Do not rely on template for pkg-config file
Review of attachment 371958 [details] [review]: This breaks translations. You move gettext files to ghex_string, but GETTEXT_PACKAGE is still PACKAGE_NAME, which is meson.project_name().
Created attachment 371979 [details] [review] Try to use consistent naming (In reply to Ting-Wei Lan from comment #15) > Review of attachment 371958 [details] [review] [review]: > > This breaks translations. You move gettext files to ghex_string, but > GETTEXT_PACKAGE is still PACKAGE_NAME, which is meson.project_name(). Fixed, thank you.
Created attachment 371980 [details] [review] Add an option for yelp manual
Created attachment 371981 [details] [review] Use smbolic links for yelp manual's media
Review of attachment 371959 [details] [review]: ::: src/meson.build @@ +2,2 @@ 'accessiblegtkhex.c', 'accessiblegtkhex.h', This can be removed as well
Review of attachment 371960 [details] [review]: ::: data/meson.build @@ -35,3 @@ - -configure_file( - input: 'gtkhex-3.pc.in', You can delete the file now.
Review of attachment 371979 [details] [review]: ::: meson.build @@ +14,3 @@ +ghex_name = meson.project_name() +ghex_string = '@0@-@1@'.format(ghex_name, ghex_version_major) Maybe use something more descriptive like ghex_pc_name @@ +19,3 @@ + +libghex_name = 'gtkhex' +libghex_string = '@0@-@1@'.format(libghex_name, libghex_version_major) Maybe use something more descriptive like libgtkhex_pc_name
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/ghex/-/issues/ Thank you for your understanding and your help.