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

change macOS < 10.12 clock to SYSTEM_CLOCK, fixes #2537 #2538

Merged
merged 6 commits into from
Apr 19, 2017

Conversation

Asmod4n
Copy link
Contributor

@Asmod4n Asmod4n commented Apr 19, 2017

This should fix the issue in #2537 and also make it work with leap seconds

@Asmod4n
Copy link
Contributor Author

Asmod4n commented Apr 19, 2017

Have found the SYSTEM_CLOCK definition way back in code from the year 2000, so it should work everywhere.

@bluca
Copy link
Member

bluca commented Apr 19, 2017

Fails to build on the Travis OSX runs:

../../src/clock.cpp:64:21: error: use of undeclared identifier 'CLOCK_MONOTONIC'
    if (clock_id != CLOCK_MONOTONIC) {

Also could you please rebase away the merge commit?

@Asmod4n
Copy link
Contributor Author

Asmod4n commented Apr 19, 2017

Strange, thought the #ifdef for CLOCK_MONOTONIC the line 154 should work, got a fix coming.

@Asmod4n
Copy link
Contributor Author

Asmod4n commented Apr 19, 2017

Doesn't the merge button have that option too? Can't find anything for it in the GitHub client

@bluca
Copy link
Member

bluca commented Apr 19, 2017

Ok, I can squash them when merging

@Asmod4n
Copy link
Contributor Author

Asmod4n commented Apr 19, 2017

Btw, std::chrono isn't useable because it would require a too new c++ Version?

@bluca
Copy link
Member

bluca commented Apr 19, 2017

Now a few tests fail:

dyld: lazy symbol binding failed: Symbol not found: __Z17alt_clock_gettimeiP8timespec
  Referenced from: /Users/travis/build/zeromq/libzmq/zeromq-4.2.3/_build/sub/src/.libs/libzmq.5.dylib
  Expected in: flat namespace

@bluca
Copy link
Member

bluca commented Apr 19, 2017

That looks like it's C++11 so yes, too new, sorry we got users on Solaris, AIX and the like

@Asmod4n
Copy link
Contributor Author

Asmod4n commented Apr 19, 2017

Saw that, sorry for that. Thought its only used in clock.cpp and not exported. Btw, why is it exported?

@bluca
Copy link
Member

bluca commented Apr 19, 2017

There's a reference in src/condition_variable.hpp as well

@bluca
Copy link
Member

bluca commented Apr 19, 2017

Originally it was without the alt_ prefix and it was just a substitute for the missing system call. Then Apple added it, and it got mangled a bit. I'll clean it up one day...

@Asmod4n
Copy link
Contributor Author

Asmod4n commented Apr 19, 2017

Oh, SublimeText hasn't found it there.
Updated it so the clock_id gets passed down to the mach function.

@@ -214,7 +214,7 @@ namespace zmq
struct timespec timeout;

#if defined ZMQ_HAVE_OSX && __MAC_OS_X_VERSION_MIN_REQUIRED < 101200 // less than macOS 10.12
alt_clock_gettime(CLOCK_REALTIME, &timeout);
alt_clock_gettime(CALENDAR_CLOCK, &timeout);
Copy link
Member

Choose a reason for hiding this comment

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

why calendar clock? I think we want monotonic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clock_gettime uses the wall clock too there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you cancel all builds then please from this pr? Ill change it all to monotonic clocks then.

Copy link
Member

Choose a reason for hiding this comment

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

thanks - this is a wrapper around pthread conditions, so monotonic clock is the right thing

Copy link
Member

Choose a reason for hiding this comment

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

killed the builds

@bluca bluca merged commit ce602d0 into zeromq:master Apr 19, 2017
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