Skip to content

Commit

Permalink
CMake: Fix Windows build with Clang/LLVM 17.
Browse files Browse the repository at this point in the history 
llvm-windres 17.0.0 has more accurate emulation of GNU windres, so
the hack for GNU windres must now be used with llvm-windres too.

LLVM 16.0.6 has the old behavior and there likely won't be more
16.x releases. So we can simply check for >= 17.0.0.

See also:
llvm/llvm-project@2bcc0fd
  • Loading branch information
@Larhzu
Larhzu committed Sep 27, 2023
1 parent 5a9af95 commit 0570308
Showing 1 changed file with 14 additions and 12 deletions.
26 changes: 14 additions & 12 deletions CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,19 +93,21 @@ set(CMAKE_MACOSX_BUNDLE OFF)
# "syntax error" from windres. Using --use-temp-file prevents windres
# from using popen() and this seems to fix the problem.
#
# llvm-windres claims to be compatible with GNU windres but with that
# the \x20 results in "XZx20Utils" in the compiled binary. (At the
# same time it works correctly with clang (the C compiler).) The option
# --use-temp-file makes no difference.
# llvm-windres from Clang/LLVM 16.0.6 and older: The \x20 results
# in "XZx20Utils" in the compiled binary. The option --use-temp-file
# makes no difference.
#
# CMake 3.25 doesn't have CMAKE_RC_COMPILER_ID so we rely on
# CMAKE_C_COMPILER_ID. If Clang is used together with GNU windres
# then it will fail, but this way the risk of a bad string in
# the binary should be fairly low.
if(WIN32 AND CMAKE_C_COMPILER_ID STREQUAL "GNU")
# Use workarounds with GNU windres. The \x20 in PACKAGE_NAME works
# with gcc too so we don't need to worry how to pass different flags
# to windres and gcc.
# llvm-windres 17.0.0 and later: It emulates GNU windres more accurately, so
# the workarounds used with GNU windres must be used with llvm-windres too.
#
# CMake 3.27 doesn't have CMAKE_RC_COMPILER_ID so we rely on
# CMAKE_C_COMPILER_ID.
if(WIN32 AND (CMAKE_C_COMPILER_ID STREQUAL "GNU" OR (
CMAKE_C_COMPILER_ID STREQUAL "Clang" AND
CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL "17")))
# Use workarounds with GNU windres and llvm-windres >= 17.0.0. The \x20
# in PACKAGE_NAME works with gcc and clang too so we don't need to worry
# how to pass different flags to windres and the C compiler.
string(APPEND CMAKE_RC_FLAGS " --use-temp-file")
set(PACKAGE_NAME "XZ\\x20Utils")
else()
Expand Down

6 comments on commit 0570308

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

Kiitos/tack!

I was just about to submit a PR doing the same, when I saw that you had updated xz already to take this into account. Very much appreciated.

When I made llvm-windres originally, I wasn't aware of how --use-temp-file affects how GNU windres mangles the command line arguments. Without the flag, the arguments get interpreted by a shell one time extra within the popen(), as you've noted. Some projects explicitly expect this - see e.g. https://git.savannah.gnu.org/gitweb/?p=gettext.git;a=blob;f=windows/windres-options;h=b12edd0edc875a15dd0b79a54b975cd6ced91b0f;hb=775d1d26a8f72214834ef4d399bad4690b7a604c#l67 - and I've made llvm-windres handle that. That works, but if you'd be using that, you'd need to pass different -D options to windres and to the compiler in general. (That setup still is problematic for passing strings with spaces in them to GNU windres though.) But using --use-temp-file seems like a much nicer solution.

FWIW, I was about to suggest a slightly different form of the cmake conditional, something like this:

if(MINGW AND (NOT CMAKE_C_COMPILER_ID MATCHES "Clang" OR
              CMAKE_C_COMPILER_VERSION VERSION_GREATER_EQUAL "17"))

This assumes that any mingw-compatible toolchain supports this, with llvm-windres older than 16 being the exception. In the future for cleanup, that distinction could also maybe be dropped entirely.

Regardless, it might make sense to change the leading if (WIN32 ... into if (MINGW in any case; I think a setup with Clang acting in MSVC mode (with llvm-rc or MS rc.exe as the RC tool) would hit this otherwise.

@Larhzu
Copy link
Member Author

@Larhzu Larhzu commented on 0570308 Oct 5, 2023

Choose a reason for hiding this comment

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

Thanks for llvm-windres and improving the GNU windres compatibility! They way GNU windres handles the arguments doesn't feel right but it cannot be fixed in backward compatible way. It was confusing to figure it out as quoting has to work in both /bin/sh and cmd.exe. The docs don't directly say that --use-temp-file affects options handling but eventually I ended up trying that too.

Using MINGW instead of WIN32: I understand the point. Would Cygwin be affected then though? I suppose Cygwin needs the windres workaround too. So should it be MINGW OR CYGWIN or WIN32 AND NOT MSVC? Or are there more targets that match WIN32 too? Sorry, I'm quite clueless here.

(There are more Windows-related improments to CMake-based build coming. I pushed some to the w32_update branch but it's not finished or polished yet. E.g. build.bash hasn't actually been tested on MSYS2 (and it's hardcoded for GCC too). Completely revised build instructions (CMake + MinGW-w64 + plain Command Prompt) for less experienced developers are coming too. For a long time I thought that CMake-based build would be for MSVC only but in the past two weeks I have started to think that it doesn't lack too many things anymore so maybe it should be polished to make it truly good on a few other common targets too.

I have heard comments saying that we should use Meson but years ago CMake support was requested due to Windows so that was started back then. Maintaining very many build systems isn't practical. Autotools cannot be dropped because those are likely the most supported method on less known platforms (for example, being EBCDIC compatible matters or at least did a few years ago) although Libtool's shared library versioning oddities have bothered me a long time.)

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

Using MINGW instead of WIN32: I understand the point. Would Cygwin be affected then though? I suppose Cygwin needs the windres workaround too. So should it be MINGW OR CYGWIN or WIN32 AND NOT MSVC? Or are there more targets that match WIN32 too? Sorry, I'm quite clueless here.

I'm not entirely 100% sure here, but I think MINGW OR CYGWIN might be the right thing; cygwin in general doesn't set WIN32 I think, as it's mostly considered unixy.

I have heard comments saying that we should use Meson but years ago CMake support was requested due to Windows so that was started back then. Maintaining very many build systems isn't practical.

Yep, totally agree. And usually it's worse if one provides multiple build systems, but their functionality isn't entirely equal, either in how the build works, or worse, the header/pkgconfig install layout differs, or ABI details like sonames or such differ between various builds.

Autotools cannot be dropped because those are likely the most supported method on less known platforms (for example, being EBCDIC compatible matters or at least did a few years ago) although Libtool's shared library versioning oddities have bothered me a long time.)

I've got plenty of Libtool frustrations myself as well...

Clang links compiler-rt builtins by passing an absolute path to the .a file, instead of passing -L + -l. If you're linking a C++ library with libtool, libtool decides to do a test run of the compiler with -v, to see what linker flags it passes by default. Then it will do the actual link with -nostdlibs and manually readd the options it thinks are relevant; it extracts all -L and -l options, but drops /absolute/path/to/lib.a. https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866 There was something that seemed like action towards getting this resolved last year, but then maintainance stopped again.

@Larhzu
Copy link
Member Author

@Larhzu Larhzu commented on 0570308 Oct 6, 2023

Choose a reason for hiding this comment

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

I'm not entirely 100% sure here, but I think MINGW OR CYGWIN might be the right thing; cygwin in general doesn't set WIN32 I think, as it's mostly considered unixy.

It's unfortunate that it's not documented very clearly but it seems that you are right. So perhaps the CMake-based build has been broken on Cygwin for quite some time.

(In C code, _WIN32 is #defined on Cygwin, which slightly adds to the confusion.)

Grepping the sources gave me a feeling that MSYS should be listed in addition to CYGWIN to cover MSYS and MSYS2.

I used STREQUAL instead of MATCHES. I suppose other Clang-based compiler strings in CMake aren't relevant in this particular case. I understand that MATCHES might be better in generic situations (it misses "IntelLLVM" though).

I have committed a fix to master. Thanks!

And usually it's worse if one provides multiple build systems, but their functionality isn't entirely equal, either in how the build works, or worse, the header/pkgconfig install layout differs, or ABI details like sonames or such differ between various builds.

Most of these apply to XZ Utils for now. w32_update has a commit to install the pkgconfig file. On GNU/Linux even that differs slightly due to CMake not adding -pthread since pthreads are in libc in modern glibc versions. That difference shouldn't matter in practice, luckily.

Soname differences are hard to avoid with Libtool vs. anything else on certain platforms.

I wonder if not supporting Meson is a problem in the long term. I have seen one or two quickly-written files for building liblzma with Meson which work for x86-64 and maybe something else but can be suboptimal or maybe even subtly broken in some other cases. Maybe it's just the way things are, upstreams cannot worry about everything.

There was something that seemed like action towards getting this resolved last year, but then maintainance stopped again.

That's a showstopper issue indeed. I didn't read in detail but I suppose there's a reason why Libtool was designed to work like it does, it's an old tool and some decisions might be less relevant on today's platforms. The big deal is that Libtool has had lack of developer resources for many years.

Thanks!

@mstorsjo
Copy link

Choose a reason for hiding this comment

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

I'm not entirely 100% sure here, but I think MINGW OR CYGWIN might be the right thing; cygwin in general doesn't set WIN32 I think, as it's mostly considered unixy.

It's unfortunate that it's not documented very clearly but it seems that you are right. So perhaps the CMake-based build has been broken on Cygwin for quite some time.

(In C code, _WIN32 is #defined on Cygwin, which slightly adds to the confusion.)

Actually, that doesn't match my experience (and I just rechecked just to be sure), AFAIK _WIN32 isn't defined by the compiler in Cygwin. (And other than that, the basics are defined in a unixy way, like sizeof(long) == sizeof(void*).)

Grepping the sources gave me a feeling that MSYS should be listed in addition to CYGWIN to cover MSYS and MSYS2.

Yep, that's probably true. (I wonder if the MSYS environment defines CYGWIN as well - I'm too lazy to check right now.) Overall very few programs care about how they work in MSYS as they primarily should be built for mingw instead. But I'm sure xz is relevant to have available in all environments.

I have committed a fix to master. Thanks!

Thanks!

Soname differences are hard to avoid with Libtool vs. anything else on certain platforms.

Yep; for one project that I maintain, I spent some extra effort in CMake to try to match the sonames that are produced with libtool closely.

I wonder if not supporting Meson is a problem in the long term. I have seen one or two quickly-written files for building liblzma with Meson which work for x86-64 and maybe something else but can be suboptimal or maybe even subtly broken in some other cases. Maybe it's just the way things are, upstreams cannot worry about everything.

I've actually been faced with that decision as well.

For http://github.com/mstorsjo/fdk-aac, where I already have autotools and CMake, I got a PR to add Meson. Out of principle, I didn't want to merge it - it doesn't give any extra platform support over autotools and CMake. And if I'd do it, I'd want to spend the same level of effort as for CMake, making it match the autotools build both wrt soname and functionality across the main platforms. But even if I didn't merge it, the incomplete/unverified/possibly subtly incompatible Meson file lives on elsewhere in their wrapdb, unmaintained by me, used by some other Meson projects. So if I'd like to make sure it works properly, I guess I should take on the maintainance of that as well.

@Larhzu
Copy link
Member Author

@Larhzu Larhzu commented on 0570308 Oct 6, 2023

Choose a reason for hiding this comment

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

Actually, that doesn't match my experience (and I just rechecked just to be sure), AFAIK _WIN32 isn't defined by the compiler in Cygwin.

OK, that's good to know, thanks. Now I wonder why the (harmless) commits 0deb1bb and 44d70cb were made and left me believe that _WIN32 is or might be defined. The commit messages are a bit confusing. :-( I remember that around those times I even tested on Cygwin.

I wonder if the MSYS environment defines CYGWIN as well

It likely does since Modules/Platform/MSYS-Initialize.cmake includes Platform/CYGWIN-Initialize which sets CYGWIN. But checking for MSYS shouldn't hurt, I hope.

But even if I didn't merge it, the incomplete/unverified/possibly subtly incompatible Meson file lives on elsewhere in their wrapdb, unmaintained by me, used by some other Meson projects.

I had to look at wrapdb now. Seems that there is XZ Utils 5.2.12 under the name liblzma but not XZ Utils 5.4.x. If I understood correctly:

  • meson.build contains the old bug reporting address (my personal email address) which was changed in 5.2.9. So it's obvious that the wrap file is updated without a throrough compare with upstream changes.

  • It assumes that the target is little endian and supports fast unaligned access.

  • Check for optreset has been commented out. OpenBSD's man page says that GNU-style optind = 0 is supported but FreeBSD and NetBSD don't. So maybe parsing the environment vars XZ_OPT and XZ_DEFAULTS is broken on those.

  • liblzma has an unneeded depedency on libintl on systems where it's not part of libc.

  • I don't see anything about installing the xz translation files even though translation support is built.

At glance I guess it is good enough in practice for the most common use cases (GNU/Linux and Windows). :-) It's clear that it has take some effort to get it to the current shape even though it would have a long way to become polished and feature complete. 5.4.x has a few new #defines so that might explain why 5.4.x isn't there yet.

Oh well. At least the windres workaround in CMake should be correct now. Thanks again!

Please sign in to comment.