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 UNIX Platform Fix #216

Merged
merged 1 commit into from
Sep 22, 2024
Merged

Conversation

bedwardly-down
Copy link
Contributor

This simple commit could solve some problems later down the road. In at least newer CMake versions, checking if the platform is UNIX like you had can cause Apple and Cygwin targets to be a miss since both count as UNIX in CMake.

See here for more information: https://cmake.org/cmake/help/latest/variable/UNIX.html

@bedwardly-down bedwardly-down changed the title CMake Platform Fix CMake UNIX Platform Fix Sep 22, 2024
@HappySeaFox
Copy link
Owner

Hi! Thanks for the PR! From my understanding, the result of this long block of "ifs" will be the same on Apple or Cygwin platforms even after merging this PR. Both the SAIL_APPLE and SAIL_UNIX will still be set on Apple platforms. So the PR doesn't alter the current behavior. Please correct me if I'm wrong.

The whole purpose of this block of "ifs" is to mimic the CMake behavior without introducing any new logic. Since on Apple platforms both the APPLE and UNIX are defined, SAIL does the same 🙂

@bedwardly-down
Copy link
Contributor Author

Right now, as is, it should be fine. Moving that to the bottom is more for readability than anything. If you change something down the line that requires one or the other being defined, though, it could help with debugging.

UNIX is a catch all for all non-Windows, non-mobile OS's / platforms (even the PS4 and PS5 count as Unix due to being built on modified BSD OS's). 😸

@HappySeaFox HappySeaFox merged commit b4a668b into HappySeaFox:master Sep 22, 2024
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