boostorg / thread

Boost.org thread module
http://boost.org/libs/thread
198 stars 162 forks source link

Fix compilation of timed functions on Cygwin #263

Closed Lastique closed 5 years ago

Lastique commented 5 years ago

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 commented 5 years ago

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

Lastique commented 5 years ago

What do you mean?

pdimov commented 5 years ago

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

Lastique commented 5 years ago

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 commented 5 years ago

Please, could you report the compilation error?

viboes commented 5 years ago

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 commented 5 years ago

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 (https://github.com/boostorg/thread/pull/244).

Lastique commented 5 years ago

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.

pdimov commented 5 years ago

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 commented 5 years ago

Could this fix be merged to master, please?

viboes commented 5 years ago

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

viboes commented 5 years ago

Andrey, please, could you provide a test case?

viboes commented 5 years ago

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