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

cmake: Fix cmake_install scripts for macOS #321

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Aug 10, 2024

The current Guix's cmake-minimal package has version 3.24.2, which has a bug that causes wrong options for llvm-strip.

This PR has exactly the same effect as the fixed CMake versions >= 3.27.

Addresses bitcoin#30454 (comment):

Had a look at a Guix build. Stripping the macOS binaries is broken:

-- Installing: /distsrc-base/distsrc-ad2140d4d8cc-arm64-apple-darwin/installed/bitcoin-ad2140d4d8cc/bin/bitcoind
/root/.guix-profile/bin/llvm-strip: error: unknown argument '-u'

Fix `cmake_install` scripts for the Darwin platform.
@hebasto hebasto added the bug Something isn't working label Aug 10, 2024
@hebasto
Copy link
Owner Author

hebasto commented Aug 10, 2024

My Guix build:

x86_64
72c94bf27482ed0442218d1bce9bc882cf92f80ffb3cca74bd53a376261de25f  guix-build-e61dbee3c2c0/output/arm64-apple-darwin/SHA256SUMS.part
654f93660b3e8108c2ca6f262b8f87cbcdb4f535eb56508f4882f3c5cbbb2443  guix-build-e61dbee3c2c0/output/arm64-apple-darwin/bitcoin-e61dbee3c2c0-arm64-apple-darwin-unsigned.tar.gz
9f940d388afb39bfb8310f9b2a18b7fbed3c1326f9e0d4767deb1fcfd04aa656  guix-build-e61dbee3c2c0/output/arm64-apple-darwin/bitcoin-e61dbee3c2c0-arm64-apple-darwin-unsigned.zip
97cf720b8bc006604e297a2e781e635e82f3b10ee93de3dd8f942e696c1cac5c  guix-build-e61dbee3c2c0/output/arm64-apple-darwin/bitcoin-e61dbee3c2c0-arm64-apple-darwin.tar.gz
e994d2052c377a8670257c12f660ab5d3098afc4faa5d669e5ce4a9f2a90f1da  guix-build-e61dbee3c2c0/output/dist-archive/bitcoin-e61dbee3c2c0.tar.gz
7e88f422d627e6bfa1bd09574d672807e383b66a17d2b9e8fb4731f496b563a7  guix-build-e61dbee3c2c0/output/x86_64-apple-darwin/SHA256SUMS.part
399e7cfc640b83b3dbad9421025a7b6fc7aa183a0de001d40b607c4501397123  guix-build-e61dbee3c2c0/output/x86_64-apple-darwin/bitcoin-e61dbee3c2c0-x86_64-apple-darwin-unsigned.tar.gz
9c27401ef081c9eab05e2ad10bcbf59af591a33e5004a1d61012e2adbeb7c636  guix-build-e61dbee3c2c0/output/x86_64-apple-darwin/bitcoin-e61dbee3c2c0-x86_64-apple-darwin-unsigned.zip
0ece9e1bd1a9cc3be3b2413bea40f3f9683dce81c47adff10485cf84ed338cf3  guix-build-e61dbee3c2c0/output/x86_64-apple-darwin/bitcoin-e61dbee3c2c0-x86_64-apple-darwin.tar.gz

@fanquake
Copy link

Should the fix be in the build system? Otherwise it's fixed for Guix, but still broken for anyone building with older versions of CMake.
Is there any other way to control/fixup the flags used? i.e Something like CMAKE_STRIP flags.
Are we working on getting the cmake-minimal or cmake versions in Guix updated? Looks like cmake is still only 3.25.1.

@hebasto
Copy link
Owner Author

hebasto commented Aug 12, 2024

Should the fix be in the build system?

Yes, ideally. But CMake doesn't provide any means to customize the strip tool invocation.

Otherwise it's fixed for Guix, but still broken for anyone building with older versions of CMake.

True. In that case, the user still able to use the strip tool directly.

Is there any other way to control/fixup the flags used? i.e Something like CMAKE_STRIP flags.

It seems possible to add flags to the CMAKE_STRIP variable. But the -u -r flags are hardcoded without usage of any dedicated variable.

Are we working on getting the cmake-minimal or cmake versions in Guix updated?

Not at the moment.

Looks like cmake is still only 3.25.1.

True.

Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK e61dbee

@theuni
Copy link

theuni commented Aug 13, 2024

Ugh so cmake install won't work for apple outside of guix? This is a really ugly fix :(

Also this from upstream CMake:

However, llvm-strip seems to produce unusable binaries in cases involving chained fixups.

We used chained fixups. With that hack, do the binaries even work?

@hebasto
Copy link
Owner Author

hebasto commented Aug 13, 2024

Ugh so cmake install won't work for apple outside of guix?

To be precise, what is broken is the --strip option in the cmake --install command for CMake < 3.27.

So, outside the Guix, the user has two options:

  • invoke the strip binary directly
  • upgrade their CMake

On macOS, the Homebrew's CMake has version 3.30.2.

This is a really ugly fix :(

I agree.

Also this from upstream CMake:

However, llvm-strip seems to produce unusable binaries in cases involving chained fixups.

We used chained fixups. With that hack, do the binaries even work?

Yes, I also saw this notice. FWIW, it is not CMake-specific. It also applies to the current Autotools-based variant.

@fanquake
Copy link

On macOS, the Homebrew's CMake has version 3.30.2.

Isn't it going to be cross-compilation (on Linux) where this is most likely to be a problem? So I don't think brews CMake is that relevant here. Also, wouldn't anyone building on macOS be using Apple strip in any case?

@hebasto
Copy link
Owner Author

hebasto commented Aug 13, 2024

On macOS, the Homebrew's CMake has version 3.30.2.

Isn't it going to be cross-compilation (on Linux) where this is most likely to be a problem?

Yes, it is.

So I don't think brews CMake is that relevant here. Also, wouldn't anyone building on macOS be using Apple strip in any case?

However, cross-compiling on macOS for another arch macOS is still a case; and I mentioned Homebrew's CMake version for that case.

@fanquake
Copy link

However, cross-compiling on macOS for another arch macOS is still a case; and I mentioned Homebrew's CMake version for that case.

Right, but that will still be using Apple strip, so I don't see why it matters here?

@hebasto
Copy link
Owner Author

hebasto commented Aug 13, 2024

However, cross-compiling on macOS for another arch macOS is still a case; and I mentioned Homebrew's CMake version for that case.

Right, but that will still be using Apple strip, so I don't see why it matters here?

Right. It doesn't matter.

@vasild
Copy link

vasild commented Aug 15, 2024

Am I right that this problem manifests only if all of the below are true:

  1. Compiling on MacOS, and
  2. Using CMake <3.27, and
  3. llvm-strip is additionally installed on the system, so CMake picks that over strip

What is the outcome when all of those are true? Does the build fail or is the error ignored? The report from https://gitlab.kitware.com/cmake/cmake/-/issues/24601 says that "Note that the build is “successful”, i.e. CPack ignores this error."

@fanquake
Copy link

fanquake commented Aug 15, 2024

Compiling on MacOS, and

No. It will primarily happen on Linux. However I assume it will happen on macOS if using LLVM tools.

What is the outcome when all of those are true?

The outcome is that CMake just ignores the failure and continues on. Which is why I assume it persisted in the Guix build for so long before anyone noticed.

@hebasto
Copy link
Owner Author

hebasto commented Aug 15, 2024

@theuni

However, llvm-strip seems to produce unusable binaries in cases involving chained fixups.

We used chained fixups. With that hack, do the binaries even work?

Yes, the binaries work. And they are stripped of the debug info the same way as on the master branch.

@vasild
Copy link

vasild commented Aug 15, 2024

Compiling on MacOS, and

No. It will primarily happen on Linux. However I assume it will happen on macOS if using LLVM tools.

Are you sure? Linux's strip does not support -u: https://www.man7.org/linux/man-pages/man1/strip.1.html so why would CMake try to use -u on Linux?

  • Linux strip does not support -u
  • llvm-strip does not support -u
  • MacOS strip does support -u

Here is a user citing some of the CMake's code which enables -u only for MacOS https://discourse.llvm.org/t/llvm-strip-r-u-and-x-on-macos/64241

Or do you mean "cross-compiling on Linux for MacOS"?

And then the outcome is that the install succeeds but the binaries are not stripped?

@fanquake
Copy link

Or do you mean "cross-compiling on Linux for MacOS"?

Yes. Hence the broken Guix build.

@vasild
Copy link

vasild commented Aug 15, 2024

Why is llvm-strip present on Guix?

@fanquake
Copy link

Because we use a Clang and LLVM based toolchain to build for macOS.

@hebasto
Copy link
Owner Author

hebasto commented Aug 15, 2024

A minor clarification: with llvm-strip: error: unknown argument '-u', the stripping is not applied, but the binaries remain functional. That’s just an excuse for why this issue was not caught earlier.

@vasild
Copy link

vasild commented Aug 15, 2024

So, this problem manifests only if all of the below are true:

  • Compiling on MacOS, or cross compiling for MacOS, and
  • Using CMake <3.27, and
  • llvm-strip is additionally installed on the system, so CMake picks that over strip

The outcome is:

  • cmake --install is unaffected
  • cmake --install --strip succeeds, but the binaries are not stripped.

ACK e61dbee

@hebasto
Copy link
Owner Author

hebasto commented Aug 15, 2024

The outcome is that the install succeeds, but the binaries are not stripped.

... with the --strip option.

Just cmake --install, without --strip, works fine.

@vasild
Copy link

vasild commented Aug 15, 2024

Updated the previous comment slightly to clarify that.

Copy link

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

Unfortunate, but I don't see another way forward here. I hope cmake learns to expose the variable for these tools, so we don't end up having to do this for more tools in the future.

Guix build:

find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
72c94bf27482ed0442218d1bce9bc882cf92f80ffb3cca74bd53a376261de25f  guix-build-e61dbee3c2c0/output/arm64-apple-darwin/SHA256SUMS.part
654f93660b3e8108c2ca6f262b8f87cbcdb4f535eb56508f4882f3c5cbbb2443  guix-build-e61dbee3c2c0/output/arm64-apple-darwin/bitcoin-e61dbee3c2c0-arm64-apple-darwin-unsigned.tar.gz
9f940d388afb39bfb8310f9b2a18b7fbed3c1326f9e0d4767deb1fcfd04aa656  guix-build-e61dbee3c2c0/output/arm64-apple-darwin/bitcoin-e61dbee3c2c0-arm64-apple-darwin-unsigned.zip
97cf720b8bc006604e297a2e781e635e82f3b10ee93de3dd8f942e696c1cac5c  guix-build-e61dbee3c2c0/output/arm64-apple-darwin/bitcoin-e61dbee3c2c0-arm64-apple-darwin.tar.gz
e994d2052c377a8670257c12f660ab5d3098afc4faa5d669e5ce4a9f2a90f1da  guix-build-e61dbee3c2c0/output/dist-archive/bitcoin-e61dbee3c2c0.tar.gz
7e88f422d627e6bfa1bd09574d672807e383b66a17d2b9e8fb4731f496b563a7  guix-build-e61dbee3c2c0/output/x86_64-apple-darwin/SHA256SUMS.part
399e7cfc640b83b3dbad9421025a7b6fc7aa183a0de001d40b607c4501397123  guix-build-e61dbee3c2c0/output/x86_64-apple-darwin/bitcoin-e61dbee3c2c0-x86_64-apple-darwin-unsigned.tar.gz
9c27401ef081c9eab05e2ad10bcbf59af591a33e5004a1d61012e2adbeb7c636  guix-build-e61dbee3c2c0/output/x86_64-apple-darwin/bitcoin-e61dbee3c2c0-x86_64-apple-darwin-unsigned.zip
0ece9e1bd1a9cc3be3b2413bea40f3f9683dce81c47adff10485cf84ed338cf3  guix-build-e61dbee3c2c0/output/x86_64-apple-darwin/bitcoin-e61dbee3c2c0-x86_64-apple-darwin.tar.gz

Also tested the zipped .app package on macos 12.7.4

@hebasto hebasto merged commit 3c200d5 into cmake-staging Aug 16, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants