Only define clock_gettime() for macOS < 10.12 #524

chrisdembia · 2016-09-21T21:11:07Z

This PR fixes #523: OSX previously did not provide clock_gettime(), but macOS 10.12 now does provide this function. This PR causes Simbody's version of clock_gettime() to only appear if using an older version of OSX.

Background on OSX SDKs, etc.

There are two versions to consider here: the MacOSX SDK version used when compiling the source code, and the version of the OS on a user's computer (called the DEPLOYMENT_TARGET). Both the SDK and the OS version use the same numbering scheme (e.g., 10.10, 10.11, 10.12). A single SDK can be used to deploy to multiple different versions. For example, one can build Simbody with the 10.12 SDK and deploy those binaries to OSX 10.10, 10.11 (as well as 10.12).

When a developer builds Simbody with CMake, they can specify:

CMAKE_OSX_SYSROOT: the SDK to use.
CMAKE_OSX_DEPLOYMENT_TARGET: the minimum version of OSX that users must have to use the resulting Simbody binaries.

The 10.12 SDK declares and defines clock_gettime(), and (as one would expect) 10.12 operating system provides clock_gettime() in a dynamic library (libSystemB.dylib maybe?).

With respect to this PR, this results in 3 scenarios:

SDK version < 10.12 and DEPLOYMENT_TARGET < 10.12: This is the old scenario. Simbody must provide clock_gettime().
SDK version >= 10.12 and DEPLOYMENT_TARGET >= 10.12: This is the scenario that will be common for most users/developers pretty soon (in say a year?). And it is simple: Simbody should not declare or define clock_gettime().
SDK version >= 10.12 and DEPLOYMENT_TARGET < 10.12: This is the nasty scenario, and it's the one I was in yesterday. Basically, the SDK declares clock_gettime(), but it may not be on the user's system. This occurs when developers update their Xcode to Xcode 8. Updating to Xcode 8 removes the previous SDKs (e.g., MacOSX10.11.sdk) and only leaves MacOSX10.12.sdk on the developer's machine. However, a developer with Xcode 8 on their machine is likely still interested in providing Simbody to users with OSX 10.11, etc. It is true that developers like me can go find MacOSX10.11.sdk online and download it again, but this is undesirable and we shouldn't force developers with Xcode 8 to fetch older SDKs.

(The scenario SDK version < 10.12 and DEPLOYMENT_TARGET >= 10.12 doesn't make sense. SDKs cannot target future versions of OSX)

Changes in this PR

This PR introduces two macros that control how Simbody builds:

(DECLARE) SimTK_IS_APPLE_AND_MUST_DECLARE_CLOCK_GETTIME: This is true if the SDK version is older than 10.12, as such SDKs do not declare clock_gettime(). The 10.12 SDK declares clock_gettime(), so we don't need to declare it again. In fact, we can't declare it again because clockid_t is different in Simbody's implementation as compared to Apple's implementation: Simbody uses typedef long clockid_t, while Apple uses an enum.
(DEFINE) SimTK_IS_APPLE_AND_MUST_DEFINE_CLOCK_GETTIME: This is true if the DEPLOYMENT_TARGET is older than 10.12: if we want to allow users with OSX 10.10 or 10.11 to use Simbody, we must define clock_gettime().

I use *_DECLARE_* to decide if we should declare clock_gettime() in Timing.h and I use *_DEFINE_* to decide if we should define clock_gettime (and related functions) in Timing.cpp.

Testing

I tested scenarios (2) and (3) above locally. On a computer with OSX 10.11, I upgraded to Xcode 8 and used the MacOSX10.12.sdk, and was able to run the tests successfully. Then I upgraded my computer to macOS 10.12 and was again able to run the tests successfully. Travis tests scenario (1), since travis is using an older OSX with an older Xcode.

macOS 10.12 contains the POSIX clock_gettime(), which we were providing previously. Now, we have logic to only define clock_gettime() on older macOS versions.

The new macro names are less ambiguous.

sherm1

Wow, that is amazingly complicated. Thanks for the great analysis and elegant solution. I made a couple of suggestions.

chrisdembia · 2016-09-22T04:40:33Z

Thank you for the quick review.

I made a couple of suggestions.

Hmm...I cannot see any suggestions.

chrisdembia · 2016-09-22T04:46:19Z

Could it be that "approving" a PR causes the other comments to not appear?

sherm1 · 2016-09-22T15:19:58Z

The new GitHub review process seems to be very casual about comments that it allegedly "batches up"! That's the second time I've lost some. Will try again.

sherm1

2nd attempt at leaving some comments!

sherm1 · 2016-09-22T15:22:22Z

SimTKcommon/include/SimTKcommon/internal/Timing.h

@@ -83,8 +117,8 @@ librt realtime library (-lrt). **/
    }
 #endif

-#if defined(_MSC_VER) || defined(__APPLE__)
-    // On Windows and OSX, the Posix clock_gettime function is missing.
+#if defined(_MSC_VER) || SimTK_IS_APPLE_AND_MUST_DECLARE_CLOCK_GETTIME


I think this is working only because || has lower precedence than && -- seems like standing on thin ice!

I added parentheses, as suggested by your other comment.

sherm1 · 2016-09-22T15:23:45Z

SimTKcommon/include/SimTKcommon/internal/Timing.h

+// number must be used instead of a macro like __MAC_10_12 because pre-10.12
+// systems won't have __MAC_10_12 defined.
+#define SimTK_IS_APPLE_AND_MUST_DECLARE_CLOCK_GETTIME defined(__APPLE__) && \
+    __MAC_OS_X_VERSION_MAX_ALLOWED < 101200 // SDK is older than 10.12.


This macro and the DEFINE one should be parenthesized to avoid precedence problems.

sherm1 · 2016-09-22T15:25:39Z

Yikes -- it took me two tries to get those two comments in! I had typed both in but only one came out when I selected "finish review".

chrisdembia · 2016-09-22T21:43:25Z

Yikes -- it took me two tries to get those two comments in!

Ah that sounds frustrating.

I have addressed your comment. Let me know if there is anything else.

sherm1

Looks great.

chrisdembia added 5 commits September 21, 2016 00:28

Remove clock_gettime() for macOS 10.12 and up.

8ac2bcd

macOS 10.12 contains the POSIX clock_gettime(), which we were providing previously. Now, we have logic to only define clock_gettime() on older macOS versions.

[travis] Test clock_gettime() changes with 10.12 SDK.

0789446

[travis] Remove xcode8 build.

540b140

Improve comments about clock_gettime().

598fd7f

Rename APPLE clock_gettime() macros.

77affb3

The new macro names are less ambiguous.

sherm1 approved these changes Sep 22, 2016

View reviewed changes

sherm1 reviewed Sep 22, 2016

View reviewed changes

Add parentheses in APPLE CLOCK_GETTIME macros.

ff9e813

sherm1 approved these changes Sep 22, 2016

View reviewed changes

sherm1 merged commit 31732fc into master Sep 22, 2016

chrisdembia mentioned this pull request Sep 22, 2016

Compilation failure with macOS 10.12 SDK #523

Closed

sherm1 deleted the macos1012_clockid_t branch September 23, 2016 03:35

chrisdembia added a commit to chrisdembia/simbody that referenced this pull request Sep 30, 2016

Mention PR simbody#524 in CHANGELOG.

ee9b58c

This was referenced Sep 30, 2016

Port macOS clock_gettime() fix to 3.5 branch #526

Merged

Mention macOS clock_gettime() fix in CHANGELOG (master branch) #527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only define clock_gettime() for macOS < 10.12 #524

Only define clock_gettime() for macOS < 10.12 #524

chrisdembia commented Sep 21, 2016 •

edited

Loading

sherm1 left a comment

chrisdembia commented Sep 22, 2016 •

edited

Loading

chrisdembia commented Sep 22, 2016

sherm1 commented Sep 22, 2016

sherm1 left a comment

sherm1 Sep 22, 2016

chrisdembia Sep 22, 2016

sherm1 Sep 22, 2016

sherm1 commented Sep 22, 2016

chrisdembia commented Sep 22, 2016

sherm1 left a comment

Only define clock_gettime() for macOS < 10.12 #524

Only define clock_gettime() for macOS < 10.12 #524

Conversation

chrisdembia commented Sep 21, 2016 • edited Loading

Background on OSX SDKs, etc.

Changes in this PR

Testing

sherm1 left a comment

Choose a reason for hiding this comment

chrisdembia commented Sep 22, 2016 • edited Loading

chrisdembia commented Sep 22, 2016

sherm1 commented Sep 22, 2016

sherm1 left a comment

Choose a reason for hiding this comment

sherm1 Sep 22, 2016

Choose a reason for hiding this comment

chrisdembia Sep 22, 2016

Choose a reason for hiding this comment

sherm1 Sep 22, 2016

Choose a reason for hiding this comment

sherm1 commented Sep 22, 2016

chrisdembia commented Sep 22, 2016

sherm1 left a comment

Choose a reason for hiding this comment

chrisdembia commented Sep 21, 2016 •

edited

Loading

chrisdembia commented Sep 22, 2016 •

edited

Loading