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

Making target detection on Mac more robust #2140

Closed
wants to merge 3 commits into from

Conversation

tdegeus
Copy link
Contributor

@tdegeus tdegeus commented Dec 30, 2020

Description

Upon encountering conda-forge/catch2-feedstock#37 if found that TargetConditionals.h may not define the used TARGET_OS_OSX, but TARGET_OS_MAC is. Since it was changed the other way before it could be safest to test for both. Not an expert here, so it would be good if an expert could comment.

I have no idea by conda-forge's CI is failing but the CI here wasn't.

I just realised that I could check if it fixes conda-forge/catch2-feedstock#37 , see conda-forge/catch2-feedstock#38 , if only I would understand how to get the right patch ;) I think that I failed because the release is not made on the main branch.

GitHub Issues

Fixes #2139

@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Merging #2140 (28bd01e) into devel (f5b413a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            devel    #2140   +/-   ##
=======================================
  Coverage   90.03%   90.03%           
=======================================
  Files         146      146           
  Lines        7089     7089           
=======================================
  Hits         6382     6382           
  Misses        707      707           

@horenmar
Copy link
Member

Thanks for the PR, I have two notes.

  1. I don't like the fallthrough-ish style. I would prefer the detection to look more like
#if (defined(TARGET_OS_X) && TARGET_OS_X == 1) ||
    (define(TARGET_OS_MAC) && TARGET_OS_MAC == 1)
#  define CATCH_PLATFORM_MAC
#elif < -- something for iphone platform -- >
  1. I would like a link to documentation or other reasonably good resource for the proper way of detecting the platforms, so that I won't have to merge a similar PR a month later 😃

@tdegeus
Copy link
Contributor Author

tdegeus commented Jan 19, 2021

Hi @horenmar . I didn't find amazing sources for the target detection, but I included one that seems the most official. For reference see also here . A note is that I never found the TARGET_OS_OSX. There was probably a reason to start using it, and it seems at least partly legal because the CI here was not failing, but it's no surprise that we got errors on conda-forge's build.

@tdegeus
Copy link
Contributor Author

tdegeus commented Jan 19, 2021

A question is: with this fix, how can be make v2.13.4 available for conda-forge? I didn't really get how the different branches are used, so I could't figure out how I could include patches in conda-forge's feedstock. Or will there simply be a patch release v2.13.5 soon, such that v2.13.4 can be skipped on conda-forge?

@horenmar
Copy link
Member

If you want the fix to go to a version in v2.x major, you need to retarget the PR to v2.x. devel is for the next major version only (I don't backport from devel to v2.x, only the other way around).

The code changes look fine, so otherwise I am ready to merge this.

@tdegeus tdegeus changed the base branch from devel to v2.x January 20, 2021 20:05
@tdegeus tdegeus changed the base branch from v2.x to devel January 20, 2021 20:06
@tdegeus
Copy link
Contributor Author

tdegeus commented Jan 20, 2021

Ok @horenmar I opened #2157 . I leave it to you if you need to merge them both

@horenmar
Copy link
Member

I am going to close this, and port it later.

@horenmar horenmar closed this Jan 21, 2021
@tdegeus tdegeus deleted the platform branch March 23, 2022 08:47
@ryandesign
Copy link

A note is that I never found the TARGET_OS_OSX. There was probably a reason to start using it,

#1084 explains why TARGET_OS_OSX was used instead of TARGET_OS_MAC.

  1. I don't like the fallthrough-ish style. I would prefer the detection to look more like
#if (defined(TARGET_OS_X) && TARGET_OS_X == 1) ||
    (define(TARGET_OS_MAC) && TARGET_OS_MAC == 1)
#  define CATCH_PLATFORM_MAC
#elif < -- something for iphone platform -- >

Even replacing the typos of TARGET_OS_X with TARGET_OS_OSX and define with defined, this change (the typo-corrected version of which was committed in bcb9ea8) doesn't make macOS detection more robust; instead, it misidentifies all non-macOS Apple operating systems as macOS.

On my Mac with macOS 12, the comments at the top of the TargetConditionals.h file explain in text and diagram form the various macros that are available and what they mean:

 *      TARGET_OS_WIN32           - Generated code will run under WIN32 API
 *      TARGET_OS_WINDOWS         - Generated code will run under Windows
 *      TARGET_OS_UNIX            - Generated code will run under some Unix (not OSX)
 *      TARGET_OS_LINUX           - Generated code will run under Linux
 *      TARGET_OS_MAC             - Generated code will run under Mac OS X variant
 *         TARGET_OS_OSX          - Generated code will run under OS X devices
 *         TARGET_OS_IPHONE          - Generated code for firmware, devices, or simulator
 *            TARGET_OS_IOS             - Generated code will run under iOS
 *            TARGET_OS_TV              - Generated code will run under Apple TV OS
 *            TARGET_OS_WATCH           - Generated code will run under Apple Watch OS
 *            TARGET_OS_BRIDGE          - Generated code will run under Bridge devices
 *            TARGET_OS_MACCATALYST     - Generated code will run under macOS
 *         TARGET_OS_SIMULATOR      - Generated code will run under a simulator
 *
 *      TARGET_OS_EMBEDDED        - DEPRECATED: Use TARGET_OS_IPHONE and/or TARGET_OS_SIMULATOR instead
 *      TARGET_IPHONE_SIMULATOR   - DEPRECATED: Same as TARGET_OS_SIMULATOR
 *      TARGET_OS_NANO            - DEPRECATED: Same as TARGET_OS_WATCH
 *
 *    +---------------------------------------------------------------------+
 *    |                            TARGET_OS_MAC                            |
 *    | +---+ +-----------------------------------------------+ +---------+ |
 *    | |   | |               TARGET_OS_IPHONE                | |         | |
 *    | |   | | +---------------+ +----+ +-------+ +--------+ | |         | |
 *    | |   | | |      IOS      | |    | |       | |        | | |         | |
 *    | |OSX| | |+-------------+| | TV | | WATCH | | BRIDGE | | |DRIVERKIT| |
 *    | |   | | || MACCATALYST || |    | |       | |        | | |         | |
 *    | |   | | |+-------------+| |    | |       | |        | | |         | |
 *    | |   | | +---------------+ +----+ +-------+ +--------+ | |         | |
 *    | +---+ +-----------------------------------------------+ +---------+ |
 *    +---------------------------------------------------------------------+

As you see, TARGET_OS_MAC encompasses macOS and every Apple OS that descended from macOS including iOS. This means that with the current code there is no way that CATCH_PLATFORM_IPHONE could ever get defined. This was already explained in #1084 in 2017.

It is true that old versions of macOS (those that predate the introduction of the iPhone in 2007) don't have TARGET_OS_OSX.

I say these things as someone familiar with macOS but unfamiliar with Catch2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TARGET_OS_OSX not defined
3 participants