Add support for building a Universal macOS app

dbry / adpcm-xq

Xtreme Quality IMA-ADPCM Encoder / Decoder

BSD 3-Clause "New" or "Revised" License

214 stars 46 forks source link

Add support for building a Universal macOS app #25

Closed gingerbeardman closed 2 months ago

gingerbeardman commented 2 months ago

I don't think this will cause any issues on other platforms.

dbry commented 2 months ago

I don't think this will cause any issues on other platforms.

If it does I'm sure we'll hear about it! :smile:

sezero commented 1 month ago

I suggest reverting this: CMAKE_OSX_ARCHITECTURES should be set by user on cmake command line and should not be shoved down the user's throat. (@madebr would know better.)

madebr commented 1 month ago

I second what @sezero said. Making it a cache variable allows overriding it by users, but in X years, when Apple deprecates x86_64 support and no longer ships x86_64 toolchains, you will have to revert this commit.

cflags (e.g. -msse4 -mavx2), ldflags (e.g. -static-libgcc -static-libstdc++) and in this instance options to configure the architectures belong in cmake toolchain files.

gingerbeardman commented 1 month ago

CMake set is meant to be used in CMakeLists.txt files
The set variable is ignored on platforms that don't support it
Having it in the CMakeLists.txt file allows hassle-free building for all users across all platforms
Mac users who want to build don't need to look anywhere else for the information
There is no other CMake toolchain file for it to be in on this project

I agree eventually it will need to be changed, if/when Apple abandon Intel and/or cmake breaks, but that's the way with software. No surprises.

So why remove it? I'm open minded.

gingerbeardman commented 1 month ago

In hindsight, I should have done:

if(APPLE)
  set(CMAKE_OSX_ARCHITECTURES arm64 x86_64)
endif()

Which might have been less offensive to the Linux crowd. Lesson learned!

madebr commented 1 month ago

This has nothing to do with Apple. The CMAKE_OSX_ARCHITECTURES variable is osx/apple specific anyways. This is about the principle that anything arch-specific should be left to the users.

For the same reasons, library authors are not adding -march=x86-64-v3 or -march=native or -msse2 -mavx2 to the compile options. This is something that should be added by the user itself, or for homebrew/Linux distros by the package maintainers.

You can't assume something that works for you is a good default for everyone.

gingerbeardman commented 1 month ago

I disagree, as you might expect.

Happy to leave it there.