Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add meson build system #2637

Merged
merged 1 commit into from
Feb 8, 2022
Merged

Add meson build system #2637

merged 1 commit into from
Feb 8, 2022

Conversation

tintou
Copy link
Contributor

@tintou tintou commented Jan 31, 2022

Allows to use the Meson build system to build the project.

Note: This PR isn't complete yet but is opened to show the interest in using the meson build system.

meson.build Show resolved Hide resolved
libvips/meson.build Outdated Show resolved Hide resolved
@jcupitt
Copy link
Member

jcupitt commented Feb 1, 2022

Hi @tintou,

Thank you for this PR.

Yes, we've been talking about switching build systems on libvips/devchat and meson is one of the front runners. If we can make this work well, we'd like to get rid of autotools completely.

Anyway, thumbs up from me and I'll try to find time to try this out.

Comment on lines +40 to +76
if (glib_dep.version().version_compare('>=2.48'))
cfg_var.set('HAVE_CHECKED_MUL', '1')
endif
if (glib_dep.version().version_compare('>=2.62'))
cfg_var.set('HAVE_DATE_TIME_FORMAT_ISO8601', '1')
endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to myself: we can also swap these defines in config.h with the GLIB_CHECK_VERSION macro. That is:

  • #ifndef HAVE_CHECKED_MUL -> #if !GLIB_CHECK_VERSION( 2, 48, 0 )
  • #ifdef HAVE_DATE_TIME_FORMAT_ISO8601 -> #if GLIB_CHECK_VERSION( 2, 62, 0 )

meson_options.txt Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
@tintou tintou force-pushed the tintou/meson branch 3 times, most recently from e52d680 to efe3b12 Compare February 1, 2022 15:13
@jcupitt jcupitt marked this pull request as ready for review February 1, 2022 15:19
@jcupitt jcupitt marked this pull request as draft February 1, 2022 15:19
test_fuzz = configure_file(
input: 'test_fuzz.sh',
output: 'test_fuzz.sh',
copy: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you actually need this file? It does nothing other than define $ASAN_OPTIONS $UBSAN_OPTIONS $VIPS_WARNING and then run each of fuzz_execs in turn. You could easily implement that directly in meson. There's an env kwarg:

https://mesonbuild.com/Reference-manual_functions.html#test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might not be needed but I'm starting with a 1:1 port and we can simplify it once it is done 😃

meson.build Outdated Show resolved Hide resolved
@tintou tintou force-pushed the tintou/meson branch 3 times, most recently from 0225ba8 to 9b8f3a0 Compare February 1, 2022 15:46
@tintou tintou force-pushed the tintou/meson branch 2 times, most recently from bc2a4d0 to 36e6270 Compare February 1, 2022 23:21
@tintou tintou force-pushed the tintou/meson branch 3 times, most recently from 3a17198 to c5f57c8 Compare February 1, 2022 23:35
@tintou
Copy link
Contributor Author

tintou commented Feb 1, 2022

I think that I got almost everything covered, I only have to add the Doxygen support and it'll be ready for review

@tintou
Copy link
Contributor Author

tintou commented Feb 2, 2022

It is now ready for review

@tintou tintou marked this pull request as ready for review February 2, 2022 09:37
@lovell
Copy link
Member

lovell commented Feb 2, 2022

Thank you Corentin, I'm +1 for having to maintain configuration for only one build tool and am happy with meson as the replacement for autotools.

To test this, I think we should ensure at least the following scripts/repos can successfully build libvips with meson/ninja.

These scripts generate various combinations of binaries, shared and static libraries, using both native and cross compilation with gcc and clang on x64 and ARM64 machines, so I'd expect them to catch most things that might be missing.

Do we want to remove autotools as part of this PR, or in a follow up?

@jcupitt
Copy link
Member

jcupitt commented Feb 2, 2022

Do we want to remove autotools as part of this PR, or in a follow up?

I don't mind. Maybe in a follow-up? It would make this PR easier to work with if it's a bit smaller.

@kleisauke
Copy link
Member

I'll test this today.

Do we want to remove autotools as part of this PR, or in a follow up?

I don't mind either. A tick–tock strategy (as noted in #2538 (comment)) is probably more appropriate for downstream packagers.

@jcupitt
Copy link
Member

jcupitt commented Feb 2, 2022

It's failing for me on ubuntu 21.10:

$ meson setup build --prefix=~/vips
Using 'PKG_CONFIG_PATH' from environment with value: '/home/john/vips/lib/pkgconfig:'
Using 'PKG_CONFIG_PATH' from environment with value: '/home/john/vips/lib/pkgconfig:'
The Meson build system
Version: 0.56.2
Source dir: /home/john/GIT/libvips
Build dir: /home/john/GIT/libvips/build
...
Run-time dependency libheif found: YES 1.11.0

meson.build:322:19: ERROR: Function does not take positional arguments.

Something odd about get_variable here?

@eli-schwartz
Copy link
Contributor

The specific manner in which it succeeds for me is instructive.

WARNING: Project specifies a minimum meson_version '>=0.56' but uses features which were added in newer versions:
 * 0.58.0: {'Positional argument to dep.get_variable()'}

kleisauke added a commit to libvips/build-win64-mxe that referenced this pull request Feb 4, 2022
meson.build Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
libvips/foreign/meson.build Outdated Show resolved Hide resolved
kleisauke added a commit to libvips/build-win64-mxe that referenced this pull request Feb 5, 2022
meson.build Outdated Show resolved Hide resolved
Allows to use the Meson build system to build the project.
subdir('doc')
endif
subdir('cplusplus')
subdir('man')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to wrap each of these last 5 subdir directives ("man" through to "fuzz") with their own boolean options, defaulting to true, to allow building only the shared/static libraries without tools and tests? This is based on a discussion at lovell/sharp-libvips#127 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see the point e.g. "man" just creates installation rules which have no build time effect, only an install-time effect. You can use meson install --tags runtime to avoid installing any files tagged as man, see https://mesonbuild.com/Installing.html#installation-tags for details.

For the other directories, they actually do create build rules, but those could for the most part be defined with build_by_default: false if you don't want to make everyone automatically build them (they could be built by name if desired).

Granted Meson currently doesn't respect build_by_default for test executables, ideally they should not be built by ninja all in that case but still get built by e.g. ninja test-prereq and pulled in as a dependency of ninja test. However, this theoretical test-prereq is instead merged into all and that's a bit annoying. We do intend to fix it, there's a WIP PR for it somewhere... in the meantime many people do have one project option "tests=false" which disables descending into tests/meson.build or what have you.

Adding 5 different options seems like a lot of cognitive overhead for very little gain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I agree, options fatigue is a real thing. Meson's use of a human-readable meson_options.txt file and its ability to fail a build when it sees unknown options is a huge improvement. I wasn't aware of the predefined --tags, very useful, thanks. I'd be happy with a single tests boolean option, which I guess would prevent descent into the test and fuzz subdirs.

kleisauke added a commit to libvips/build-win64-mxe that referenced this pull request Feb 8, 2022
if librsvg_dep.version().version_compare('>=2.46')
cfg_var.set('HAVE_RSVG_HANDLE_RENDER_DOCUMENT', '1')
endif
zlib_dep = dependency('zlib', version: '>=0.4', required: get_option('zlib'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add zlib_dep = disabler() outside this if-statement as well. Otherwise it would fail when configuring with -Drsvg=disabled (noticed this on the WebAssembly build of libvips).

libvips/include/vips/meson.build:86:0: ERROR: Unknown variable "zlib_dep".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A disabler is functionally identical to dependency('', required: false) except for the special feature of disabling any meson.build statement that tries to use it directly rather than via the .found() method.

This doesn't directly matter in the current PR, but may or may not be desirable in the general sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In meson 0.59.0, you can use this: https://mesonbuild.com/Reference-manual_returned_feature.html#featurerequire

want_zlib = get_option('zlib').require(
    librsvg_dep.found() and cairo_dep.found(),
    error_message: 'librsvg and cairo not found'
)

This would then error out if -Dzlib=enabled with the error message:

Feature zlib cannot be enabled: librsvg and cairo not found

Then you just search for zlib in the main flow, not inside an if block.

Currently if you define zlib as enabled but librsvg and/or cairo are optional + they aren't found, zlib simply gets ignored. Alas, that's the best you can do while still building with the version of Meson available on Debian stable. Perhaps add a comment to migrate to .require() once the minimum version of Meson is updated?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for a comment to migrate to .require() once we upgraded our minimum version to Meson 0.59.

Note to myself: the module logic here:

if gmodule_dep.get_variable(pkgconfig: 'gmodule_supported') != 'false'
    modules_enabled = get_option('modules').enabled() or get_option('modules').auto()
elif get_option('modules').enabled()
    error('GModule is not supported on your system, please reconfigure with -Dmodules=disabled')
else
    modules_enabled = false
endif

could also be replaced with:

modules_enabled = get_option('modules').require(
    gmodule_dep.get_variable(pkgconfig: 'gmodule_supported') != 'false',
    error_message: 'GModule is not supported on your system, please reconfigure with -Dmodules=disabled
).allowed()

by then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ha, that code is even more gnarly, fortunately Meson 0.59 can save us from complicated scripting logic.

]

foreach tool : tools
executable(tool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is missing here.

script_data = configuration_data()
script_data.set('prefix', get_option('prefix'))
foreach script : scripts
configure_file(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

kleisauke added a commit to kleisauke/wasm-vips that referenced this pull request Feb 8, 2022
Copy link
Member

@kleisauke kleisauke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The final comments can also be incorporated later.

cc = meson.get_compiler('c')
cpp = meson.get_compiler('cpp')

glib_dep = dependency('glib-2.0')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to myself: minimum required GLib version is missing here.

Suggested change
glib_dep = dependency('glib-2.0')
glib_dep = dependency('glib-2.0', version: '>=2.40')

Copy link
Member

@jcupitt jcupitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't approve earlier. Looks good for my uses.

Copy link
Member

@lovell lovell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@jcupitt jcupitt merged commit 0290143 into libvips:master Feb 8, 2022
@jcupitt
Copy link
Member

jcupitt commented Feb 8, 2022

OK, let's merge!

Thank you very much for doing all this work, @tintou -- it's great improvement (something we've been avoiding for years) and I expect it'll be the main feature of 8.13. I'll add your name to the changelog with a large credit (hope that's OK).

And thank you for your advice and assistance @eli-schwartz!

jcupitt added a commit that referenced this pull request Feb 8, 2022
@jcupitt
Copy link
Member

jcupitt commented Feb 8, 2022

I made an issue for tracking any small snags we find with the new build system here: #2650

Let's shift any discussion to that issue (this PR is large enough already).

@eli-schwartz
Copy link
Contributor

It is my pleasure to offer advice and reviews to people migrating to / updating their usage of meson. :)

(Seriously. Feel free to @ me in issues/PRs with your meson-related questions.)

@yisibl
Copy link

yisibl commented Feb 19, 2022

Does this mean we can more easily compile statically linked libvips?

When I try to cross-compile sharp with zig in centos7, I get the error:

CC="zig cc -target x86_64-macos" CXX="zig c++ -target x86_64-macos" npx node-gyp rebuild

COPY Release/nothing.a
  CXX(target) Release/obj.target/sharp-linux-x64/src/common.o
../src/common.cc:24:10: fatal error: 'vips/vips8' file not found
#include <vips/vips8>
         ^~~~~~~~~~~~
1 error generated.
make: *** [Release/obj.target/sharp-linux-x64/src/common.o] Error 1

I guess this is because the libvips that sharp relies on are dynamically linked.

@jcupitt
Copy link
Member

jcupitt commented Feb 19, 2022

You can make a static library very simply with most build systems. With autotools try:

./configure --enable-static --disable-shared

With meson it's:

meson setup build --default-library=static

Then compile a program with eg.:

gcc -g -Wall myprog.c -Wl,-Bstatic `pkg-config vips --cflags --libs`

I think you'll find the difficulty comes in making static versions of all the things that libvips depends on.

@yisibl
Copy link

yisibl commented Feb 19, 2022

@jcupitt Thanks for your reply, I found someone using staticx to do this, can you see if this way can make libvips dependencies static?

https://github.com/whalehub/libvips-static/blob/31a21dadca84f3f9e3c568650f3844feaea5cb29/Dockerfile#L103

@jcupitt
Copy link
Member

jcupitt commented Feb 19, 2022

staticx doesn't use static libraries, confusingly -- it's more like an executable zip file. You should be able to use it on any vips binary.

That dockerfile is interesting, I'll try it here.

@eli-schwartz
Copy link
Contributor

eli-schwartz commented Feb 20, 2022

I alluded to it earlier in this PR, but you can make static versions of all the things libvips depends on, using Meson subprojects:

for i in expat glib lcms2 libexif libjpeg-turbo libpng libtiff spng zlib; do 
    meson wrap install "$i"
done

And while this doesn't cover every one of libvips' dependencies, it covers a bunch of them and more could be added to the wrapdb.

There is one slight flaw: checking for gmodule_supported doesn't (currently) work from a glib subproject, since the latest stable release of glib has to support versions of Meson too old to have support for defining variables in a declare_dependency. And this project is slightly too backwards compatible to support getting that variable anyway.

The meson.build could be slightly tweaked to not actually check this in the -Dmodules=disabled case. Basically change:

-gmodule_dep = dependency('gmodule-no-export-2.0')
+gmodule_dep = dependency('gmodule-no-export-2.0', required: get_option('modules'))

And change:

if gmodule_dep.get_variable('gmodule_supported') != 'false'
    modules_enabled = get_option('modules').enabled() or get_option('modules').auto()
elif get_option('modules').enabled()
    error('GModule is not supported on your system, please reconfigure with -Dmodules=disabled')
else
    modules_enabled = false
endif

to (first two lines changed)

if gmodule_dep.found() and gmodule_dep.get_variable('gmodule_supported') != 'false'
    modules_enabled = true
elif get_option('modules').enabled()
    error('GModule is not supported on your system, please reconfigure with -Dmodules=disabled')
else
    modules_enabled = false
endif

It's normally guaranteed to be found if glib is found at all. But if you explicitly disable it, then you:

  1. don't need to bother linking to it when it isn't used (ENABLE_MODULES=0)
  2. don't need to check its variables

@jcupitt
Copy link
Member

jcupitt commented Feb 20, 2022

@eli-schwartz huh that's interesting. It sounds like it'd be useful a little way down the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants