search

tukaani-project / xz

XZ Utils
https://tukaani.org/xz/
Other
561 stars 104 forks source link

CMake configure error in case '-DENABLE_NLS=ON' is given and cmake version is to old (e.g. 3.18.4) #129

Closed pseiderer closed 3 months ago

pseiderer commented 3 months ago

CMake configure error incase '-DENABLE_NLS=ON' is given and cmake version is to old (e.g. 3.18.4):

  CMake Error at CMakeLists.txt:1675 (add_executable):
    Target "xz" links to target "Intl::Intl" but the target was not found.
    Perhaps a find_package() call is missing for an IMPORTED target, or an
    ALIAS target is missing?

  CMake Error at CMakeLists.txt:1619 (add_executable):
    Target "lzmainfo" links to target "Intl::Intl" but the target was not
    found.  Perhaps a find_package() call is missing for an IMPORTED target, or
    an ALIAS target is missing?
Larhzu commented 3 months ago

I suppose it's not the best if user explicitly enables NLS and that gets silently ignored. Here the current code is buggy with new CMake too. I try to make an alternative fix proposal.

Larhzu commented 3 months ago

Please checkout fb99f8e8c50171b898cb79fe1dc703d5f91e4f0a in the cmake_nls_fixes branch. It changes the default behavior a little too. The default is still a bit weird as it depends on whether Intl is found or not. But perhaps it's better this way still. In any case, it should fix bugs when user sets XZ_NLS=ON but when it cannot actually work due to too old CMake or missing Intl.

I would like to hear your opinion if this is OK/better/worse than your suggestion.

I'm also pondering if support for pre-generated .gmo files should be dropped. It's very little code and simple code too, but .gmo files are binary files that get installed. If people can install CMake, I suppose they can install gettext tools too. Shipping pre-generated .gmo files feels like Autotools-specific thing.

Larhzu commented 3 months ago

A few days ago I posted in the other issue if it was OK to require CMake 3.20. I suppose there was a reason why you tested with an older version, so it would be valuable to hear how inconvenient it would be to you if 3.20 was required. Thanks!

pseiderer commented 3 months ago

Yep, branch cmake_nls_fixes works for me (and a warning/error is definitely better)...

I believe dropping pre-generated .gmo files and gettext support would be o.k. for the Buildroot target package build (my use case)...

Require CMake 3.20 would be a bit problematic as in Buildroot there is not (yet) support for host packages to require a minimum CMake version (for the target packages support is present)...

The failure comes from the default Buildroot docker/podman (CI) container, see (for the whole story): https://lore.kernel.org/buildroot/20240702144718.7a91e116@gmx.net/ https://lore.kernel.org/buildroot/20240612135727.11811-1-ps.report@gmx.net/T/#u

pseiderer commented 3 months ago

Needed the following change to avoid this build failure (on branch branch cmake_nls_fixes with buildroot in docker/podman container - Debian GNU/Linux 11 (bullseye)):

/usr/bin/ld: CMakeFiles/xz.dir/src/xz/signals.c.o: undefined reference to symbol 'pthread_sigmask@@GLIBC_2.2.5'
/usr/bin/ld: /lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line
index 5e294530..219c832f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -642,7 +642,7 @@ if(XZ_THREADS)
             # liblzma-config.cmake later.
             set(USE_POSIX_THREADS ON)

-            target_link_libraries(liblzma PRIVATE Threads::Threads)
+            target_link_libraries(liblzma PUBLIC Threads::Threads)
             add_compile_definitions(MYTHREAD_POSIX)

             # Make the thread libs available in later checks. In practice
Larhzu commented 3 months ago

I believe I fixed the pthread_sigmask linker error in cmake_nls_fixes although I didn't test it on an affected system.

One doesn't want to use PUBLIC as that exports slightly more than is strictly required. This way other packages that use liblzma::liblzma don't get pthread flags when they don't need it.

Larhzu commented 3 months ago

I merged cmake_nls_fixes to master.

While I thought that in 5.6.2 the CMake support would be fairly ready, in reality it unfortunately seems that there are quite a few small issues, some even on Windows for which CMake support was initially added.

The changes to CMake support in master are so significant that perhaps those shouldn't be backported to 5.6.x though (config variables were renamed, their args changed, i386 asm handling is good now, all tests are now run, not marked experimental anymore). These changes have introduced new bugs too like the pthread_sigmask issue you found but overall I think the changes are a significant improvement. In master the CMake support should finally be equal enough with Autotools, including the options that are missing in 5.6.2.

Also, the package-specific options are (mostly) documented in INSTALL now. However, I'm aware that the docs in general need some updating here and there.

I believe dropping pre-generated .gmo files and gettext support would be o.k. for the Buildroot target package build (my use case)...

Thanks. I suppose it doesn't need NLS support, so it doesn't matter much in Buildroot.

Require CMake 3.20 would be a bit problematic as in Buildroot there is not (yet) support for host packages to require a minimum CMake version (for the target packages support is present)...

Thanks. I guess that would be a solvable problem if Buildroot might need to build a fresh CMake for cross-compilation use anyway.

The minimum version is a bit tricky question as requiring a newer one would clean up the code slightly (not a lot but still) and it would avoid different behavior depending the CMake version. The latter made it difficult for you to notice the NLS bug because it only affected old CMake versions and only if NLS was explicitly enabled. But the upside was that I spotted other bugs as a side effect of your bug report.

Some random comments about the linked messages:

And the amount of work done for the cmake build system seems to hint that xz is moving away from autoconf/automake towards the cmake system...

Actually there is a note on the home page about future plans but those might already be a bit stale. I got an impression that vcpkg really likes CMake and perhaps there are other users too. Meson support is indeed wished quite strongly. A few comments I got were such that of these three, Autotools is the most convenient in certain cases because of circular dependencies.

Perhaps bootstrapping is mostly a problem for source-based distros. But Autotools is also convenient for obscure or old systems where CMake might be difficult to install.

So, who knows, perhaps there will be three build systems without dropping one of them. Clearly this project is becoming a build system playground. It's OK to me, but I see that the status of the build systems should be communicated better.

+XZ_CONF_OPTS = \

  • --enable-encoders=lzma1,lzma2,delta,x86,powerpc,ia64,arm,armthumb,arm64,sparc,riscv \

It feels a bit scary to see all options listed this way. PACKAGERS tries to list options that are only for embedded use or other special cases. And yes, I know that Buildroot is for embedded use. :-) If the intent is to list options so that people know what they can disable, then it can be fine. One just needs to be really careful to keep the list up to date as an old list likely won't fail the build but it might make it deficient.

Thanks for your help!

thesamesam commented 3 months ago

Maybe to say explicitly: we don't expect people to port from autotools->CMake right now unless they want to, and we'll make clear if/when something is being dropped.

We're particularly glad you ended up trying it because you found several issues here, but please don't think you're expected to migrate either.

I also don't know if we're going to backport all the fixes for CMake to 5.6.x or not. It's a lot of churn still.

pseiderer commented 3 months ago

I believe I fixed the pthread_sigmask linker error in cmake_nls_fixes although I didn't test it on an affected system.

One doesn't want to use PUBLIC as that exports slightly more than is strictly required. This way other packages that use liblzma::liblzma don't get pthread flags when they don't need it.

Tested for my use case (buildroot docker/podman compile), works! Thank you very much for the quick fix!

pseiderer commented 3 months ago

And thanks for the in-depth insight on the CMake/meson build system and packaging! Will retry with the next major xz release ;-)