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

Fix compilation of timed functions on Cygwin #263

Merged
merged 2 commits into from
Jan 6, 2019

Conversation

Lastique
Copy link
Member

@Lastique Lastique commented Jan 6, 2019

Cygwin includes both Windows and POSIX API and, depending on the included headers, may have WIN32 macros defined. Since Boost.Thread uses pthread API on Cygwin, it should also enable POSIX API for time units (i.e. timespec).

Cygwin includes both Windows and POSIX API and, depending on the included headers, may have WIN32 macros defined. Since Boost.Thread uses pthread API on Cygwin, it should also enable POSIX API for time units (i.e. timespec).
@viboes
Copy link
Collaborator

viboes commented Jan 6, 2019

We should the same as we do for Boost.Chrono, isn't it?

@Lastique
Copy link
Member Author

Lastique commented Jan 6, 2019

What do you mean?

@pdimov
Copy link
Member

pdimov commented Jan 6, 2019

This should probably use whatever actual threading API has been selected (via b2 threadapi=...).

@Lastique
Copy link
Member Author

Lastique commented Jan 6, 2019

That feature is currently unrelated to BOOST_THREAD_CHRONO* macros, which seems to be a configuration point of their own. The problem is that on Cygwin the library uses pthread by default, but doesn't compile the timespec-related glue, which breaks the compilation.

@viboes
Copy link
Collaborator

viboes commented Jan 6, 2019

Please, could you report the compilation error?

@viboes
Copy link
Collaborator

viboes commented Jan 6, 2019

I believe that originally even on Cygwin Boost.Thread didn't used the pthread api.
I don't remember since when it has been moved to use by default the pthread API.

@pdimov
Copy link
Member

pdimov commented Jan 6, 2019

What compilation does it break? I'm pretty sure we had the tests passing under Cygwin at one point.

The default under Cygwin is pthread, but you can use threadapi=win32, and I think I got the tests passing under that configuration too (#244).

@Lastique
Copy link
Member Author

Lastique commented Jan 6, 2019

It breaks Boost.Log with errors like this:

In file included from ./boost/thread/recursive_mutex.hpp:16:0,
                 from ./boost/log/sinks/sync_frontend.hpp:33,
                 from ./boost/log/utility/setup/file.hpp:32,
                 from libs/log/test/compile/self_contained_header.cpp:17:
./boost/thread/pthread/recursive_mutex.hpp: In member function �bool boost::recursive_timed_mutex::do_try_lock_until(const internal_platform_timepoint&)’:
./boost/thread/pthread/recursive_mutex.hpp:350:77: error: �const internal_platform_timepoint {aka const struct boost::detail::mono_platform_timepoint}’ has no member named �getTs’; did you mean �getNs’?
                 int const cond_res=pthread_cond_timedwait(&cond,&m,&timeout.getTs());
                                                                             ^~~~~
                                                                             getNs

I've added a commit to select the timing API based on the threading API.

@viboes viboes merged commit 800d0ca into boostorg:develop Jan 6, 2019
@Lastique Lastique deleted the patch-6 branch January 6, 2019 19:51
@pdimov
Copy link
Member

pdimov commented Jan 6, 2019

I tried the tests under Cygwin (before this patch), and they all pass both with threadapi=win32 and without. So this issue had no test coverage, FWIW.

@Lastique
Copy link
Member Author

Could this fix be merged to master, please?

@viboes
Copy link
Collaborator

viboes commented Jan 18, 2019

I suspect that Peter is asking for a test case that probes there is an issue, isn't it?

@viboes
Copy link
Collaborator

viboes commented Jan 19, 2019

Andrey, please, could you provide a test case?

Lastique added a commit to Lastique/thread that referenced this pull request Jan 19, 2019
The new set of tests iterates over Boost.Thread public headers and verifies that
each header is self-contained, i.e. doesn't have any missing includes and
contains syntactically correct content. On Windows and Cygwin, a second test
per each header is generated, which includes the header after windows.h. This
verifies that the header doesn't have conflicts with Windows SDK and that
includeing windows.h does not break platform detection in Boost.Thread.

This set of tests would have detected the bug fixed by
boostorg#263.
@viboes
Copy link
Collaborator

viboes commented Jan 19, 2019

Thanks, I will do the merge as soon as possible.

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.

3 participants