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

be more sophisticated about deriving configs from macOS variables #310

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

stuij
Copy link
Member

@stuij stuij commented Sep 25, 2023

edit:
current commit message:

this patch is fixing two issues:

Currently when figuring out the toolchain package name, we make some wrong
assumptions like the presence of CMAKE_OSX_ARCHITECTURES means a unified
build. Also when CMAKE_OSX_ARCHITECTURES is not present and we are building on
macOS silicon, the package name will be `AArch64`.

Should do some parsing of the CMAKE_OSX_ARCHITECTURES and look at the
CMAKE_SYSTEM_PROCESSOR var, to deduce the right system name.

Basically when CMAKE_OSX_ARCHITECTURES is set and non-empty, this overrides the
system architecture, and we need to figure out if we do a unified build (both
x86_64 and arm64 are specified), or either.

If it's a unified build, use `unified` to make this clear, instead of the
current behaviour of omitting the arch string.

Another issue is that we use the presence of the CMAKE_OSX_ARCHITECTURES string
to infer if we should build a `.dmg` or not. Seeing the complexity around
CMAKE_OSX_ARCHITECTURES, it's better to just use CMAKE_SYSTEM_NAME, which would
always point to `Darwin`"

@stuij
Copy link
Member Author

stuij commented Sep 25, 2023

I'll redo this patch in the next few days to do better OSX input checking. Currently in the CMakeLists.txt file ‘CMAKE_OSX_ARCHITECTURES’ is used as a synonym for universal library, which it isn’t. Users can of course put whatever they like in it, and I think it’s totally ignored when used on a non-Apple arch. It's very common to just put the system arch of the build machine in there. So really what we should do is check both CMAKE_OSX_ARCHITECTURES and the system arch/OS variables, and make meaningful conclusions.

@stuij stuij changed the title handle CMake configurations without CMAKE_OSX_ARCHITECTURES be more sophisticated about deriving configs from macOS variables Sep 26, 2023
Copy link
Contributor

@voltur01 voltur01 left a comment

Choose a reason for hiding this comment

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

Hi Ties, thank you for looking into this and updating the patch! Please see a few comments from my side.

this patch is fixing two issues:

Currently when figuring out the toolchain package name, we make some wrong
assumptions like the presence of CMAKE_OSX_ARCHITECTURES means a unified
build. Also when CMAKE_OSX_ARCHITECTURES is not present and we are building on
macOS silicon, the package name will be `AArch64`.

Should do some parsing of the CMAKE_OSX_ARCHITECTURES and look at the
CMAKE_SYSTEM_PROCESSOR var, to deduce the right system name.

Basically when CMAKE_OSX_ARCHITECTURES is set and non-empty, this overrides the
system architecture, and we need to figure out if we do a unified build (both
x86_64 and arm64 are specified), or either.

If it's a unified build, use `unified` to make this clear, instead of the
current behaviour of omitting the arch string.

Another issue is that we use the presence of the CMAKE_OSX_ARCHITECTURES string
to infer if we should build a `.dmg` or not. Seeing the complexity around
CMAKE_OSX_ARCHITECTURES, it's better to just use CMAKE_SYSTEM_NAME, which would
always point to `Darwin`
Copy link
Contributor

@voltur01 voltur01 left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the comments!

@stuij stuij merged commit 5040a95 into ARM-software:main Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants