-
Notifications
You must be signed in to change notification settings - Fork 43
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
Meson build #22
base: master
Are you sure you want to change the base?
Meson build #22
Conversation
Got this on Mac
All tests were passing though Here's the meson-generated pkgconfig
|
Doesn't sound like an improvement over the old system (so far at least)... I know 1 person complained, but for everyone else autotools seems to work OK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, nice to see you considering meson. Some suggestions for potential optimizations:
meson.build
Outdated
|
||
# Detect for iconv | ||
compiler_has_iconv = compiler.has_function('iconv_open') | ||
iconv_pkgconfig = get_option('iconv_pkgconfig') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm??? Which system has a pkg-config file for iconv?
For the record, as I've just added mesonbuild/meson#9205 to meson, this should work on (unreleased) meson 0.60 out of the box, but it will do the has_function check for you followed by looking for compiler.find_library('iconv')
So this entire section could be reduced to one line, but you'll need to target meson >= 0.60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the flag is misnamed, it's more like use Meson's resolver to find iconv (which is required on a Mac - FindIconv is needed)
I think it should be harmless to remove the flag entirely and always use the auto
path here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does FindIconv do anything above and beyond detecting the existence of a find_library('iconv')
?
Because having this detection not work on systems without cmake installed, is unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since iconv doesn't come with pkg-config, and we only check for builtin iconv_open
here, it doesn't work on a Mac which needs -liconv
to link. FindIconv does give us the flags we need, so it essentially serve as the pkg-config file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but compiler.find_library('iconv')
also gives you -liconv
without using cmake to look it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out mingw-w64 does ship iconv. I guess I have to add back dependency support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That pkg-config file appears to come from the msys2 packaging, not upstream iconv. And it returns the same information that find_library() returns.
I would not bother using it since it's not an official pkg-config file.
Updated
Test build was done in Arch Linux & Mac (without CMake installed) |
datrie/meson.build
Outdated
link_depends : link_depends, | ||
version : meson.project_version(), | ||
include_directories : configuration_inc, | ||
vs_module_defs : 'libdatrie.def', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This break Windows gcc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vs module defs file? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would supply the .def file generated in the block for msvc above (I think I forgot to change this line to reference the generated file). However, the original def file here got passed to gcc which breaks.
I'm not sure why - I'm not exactly familiar with msvc/mingw (and it seems this project doesn't run on Windows either) I'm guessing that both compilers use the same linker def format on Windows? Maybe if I supply the generated file to gcc it might work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, you'll need the link_def
variable reference which contains a custom_target output.
I'm not actually sure I understand the custom_target? Is it just prepending a single constant line? Why isn't this changed in the source file so that you don't need to generate anything?
Also yes, the GNU compiler implementation in meson passes the defs file to the linker as this is valid there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is passing a single line. I'm not sure how the original file is used, but it was used as-is in autotools (and I don't know which platform it was for - on Linux it use .map) so I didn't want to touch it. On Windows you need a section marker so I took inspiration from #15 to add it during build time.
Thank you this solved my problem, with #29 |
First of all, thank you very much for your work, and sorry for a very slow response. It took me a long while to find some time to study meson so I can understand your proposal. Here are what I noticed:
soversion = LT_CURRENT - LT_AGE
libversion = '@0@.@1@.@2@'.format(soversion, LT_AGE, LT_REVISION)
When the meson build is ready, I think we can move away from autotools and maintain only one build system in the end. |
It includes the output of running autogen.sh which is arguably important to ensure that Arguably .cvsignore could just be deleted from the repo ;) and the gitignore file can be excluded using gitattributes export-ignore. |
The decent outputs of
No doubt for this. |
So what's left for this PR to be merged? I might be able to work on it right away but would be great if I could have a list. From what I understand:
I don't know about the current state of Windows builds so I'm not sure if the discussion with @eli-schwartz above is still valid or not. Personally I don't want to hand test all the OSes again (I don't have access to any Mac now) so I'd prefer those test to be done by GitHub Actions. I could contribute one later if this get merged. |
There's nothing wrong with doing this. project(...,
version: run_command('./git-version-gen', check: true).stdout().strip() Note that it will only be run on configure, so if the output changes after a git pull a new configure won't be done and the version number becomes outdated. vcs_tag() can be used to generate a version.h file as a build edge (produced every time you run ninja) if that's an unacceptable shortcoming. But it depends on whether the project version is used for anything at build time other than e.g. the configure summary text, and whether you are actually worried if it gets out of date. Some people are, some people aren't. |
Updated
I also tested the build with muon and the test passes. It doesn't seems to implement the same iconv check as meson, but at least on Linux-glibc one doesn't need special flags for iconv anyway. |
I am on the latest Linux Mint, it has Meson 0.61.2... But soon I hope there will be a new release. |
I'm testing the build on Windows and the code build just fine in both MSVC project build, ninja build and MINGW64. I've also fixed building with Clang that comes with Visual Studio. However, some test fails for all variants. If someone is trying to build this, install Git and Visual Studio Community 2022 with Desktop development with C++ workload. You'll need to run Meson in Git bash since we use On investigation of the MSVC version, It seems that |
Co-authored-by: Eli Schwartz <[email protected]>
Updated a bit to fix bad merge, and updated subproject usage to automatically override parent project dependency & trietool for better subproject UX. |
Per my offer on Mastodon, here's a Meson build implementation for libdatrie. Hopefully closes #13, as it should be less painful than autotools but it's not CMake.
What works:
Needs to know for maintainer:
/meson.build
line 1. This version is used as .so suffix.-liconv
detection. Instead, it would try building without flags, and then use meson dependency resolution to find iconv. Notably, on Mac it use FindIconv from CMake, so it is a compile time dependency on the Mac.libdatrie/configure.ac
Line 36 in fb3c883
What needs to be tested: