Concept ACK.

However, an alternative solution seems to be to just use gmtime_s()
instead, when compiling with mingw-w64, as gmtime_r() just wraps gmtime_s() anyways.

Does this alternative solution has any downsides?

Concept ACK

Gitian builds

Did a bit more digging. Here's what I found:

This commmit is the one that caused the change in behaviour.

There's a very short mailing list thread about it here.

My conversation with the author of the commit on IRC:

15:02 <dongcarl> jacekc: After your "Don't define _POSIX_THREAD_SAFE_FUNCTIONS." patch, which landed in 7.0.0, what is the right way to get `gmtime_r` in my application?
15:02 * dongcarl waits patiently
15:04 <jacekc> dongcarl: ideally, I'd suggest to use gmtime_s
15:04 <dongcarl> jacekc: Why's that?
15:05 <jacekc> because that's what the platform you target offers
15:06 <jacekc> but if you prefer mingw wrappers, you may define _POSIX_THREAD_SAFE_FUNCTIONS or _POSIX_C_SOURCE
15:07 <dongcarl> jacekc: I think I see what you're saying. I'm assuming the mingw wrappers don't offer thread safety, even though the name is gmtime_r?
15:07 * dongcarl is thankful for the patient response
15:09 <jacekc> both _s and _r variants offer thread safety
15:09 <jacekc> _r variant is just an inline wrapper in time.h aroud _s
15:12 <dongcarl> jacekc: I see. Out of curiosity, what's the difference between defining _POSIX_THREAD_SAFE_FUNCTIONS and _POSIX_C_SOURCE? Feel free to just send me a link to read
15:14 <jacekc> _POSIX_THREAD_SAFE_FUNCTIONS enables only a few inline functions in time.h, _POSIX_C_SOURCE enables much more (most notably mongw stdio implementation)
15:15 <jacekc> I don't know if it's documented anywhere, but it's easy to find in the sources
15:16 <dongcarl> jacekc: Thanks for your help. Very much appreciated and have a tremendous day!
15:16 <jacekc> you welcome

Here's some more general use information on _POSIX_C_SOURCE and _POSIX_THREAD_SAFE_FUNCTIONS.

Looks like this makes the include order of headers more resilient and fixes a bug I was running in to a year ago: https://github.com/bitcoin/bitcoin/pull/16046/files#r285601479

If you feel like, you might sort the includes in the header now:

diff --git a/src/util/time.h b/src/util/time.h
index 77de1e047d..a8c06ef350 100644
--- a/src/util/time.h
+++ b/src/util/time.h
@@ -6,9 +6,9 @@
 #ifndef BITCOIN_UTIL_TIME_H
 #define BITCOIN_UTIL_TIME_H
 
+#include <chrono>
 #include <stdint.h>
 #include <string>
-#include <chrono>
 
 void UninterruptibleSleep(const std::chrono::microseconds& n);
 

If I'm thinking about this the right way, your diff implies that previously the #ifdef _MSC_VER would fail, and branch to the #else, which used gmtime_r, which would be fine because it was defined by default in mingw-w64.

If #ifdef _MSC_VER is how we detect if we're building for windows, and it's failing, then perhaps there are other places in the codebase where this detection needs to be fixed?

If you feel like, you might sort the includes in the header now:

Done.

If #ifdef _MSC_VER is how we detect if we're building for windows, and it's failing, then perhaps there are other places in the codebase where this detection needs to be fixed?

I've looked into this, and there are some cases where we are using _MSC_VER where we might be running into unexpected behavior. I don't necessarily want to address that as part of this PR, but will definitely be following up.

Something has changed in the mingw-w64 headers such that we
no-longer compile when using 7.0.0.

Interesting why master could be built successfully with Ubuntu's g++-mingw-w64 7.3.0?

Interesting why master could be built successfully with Ubuntu's g++-mingw-w64 7.3.0?

The version of mingw-w64 being used is 5.0.0. 7.3.0 is the version of GCC.

From #bitcoin-builds meeting notes today: We should investigate how to detect features (i.e. check for function availability at configure time), rather than detecting platforms. This might be more robust.

@dongcarl Is this a general advise/issue or a blocker on this specific patch set?

@MarcoFalke I think it's a blocker while we investigate if feature detection is the right path take going forward, no point merging this then changing right after (unless the release schedule dictates?)

Here's an alternative: master...Empact:2020-03-gmtime-config

Note I haven't run it on windows/mingw, and I'm not sure how to get the test to distinguish between MSC-style gmtime_s and C-standard-style gmtime_s - i.e. I doubt the existing casting-based approach.

Edit: and another more succinct alternative: master...Empact:2020-03-gmtime-check-funcs

Inspired by sqlite's handling of the same question: https://github.com/sqlite/sqlite/blob/118efd162632298bccba21b71934f666e556f594/configure.ac#L111

Although they fall back to gmtime, so it's not directly comparable.

ACK 1b801e770477e756d67402970694bca73fa0ac7f.

Using the windows function on windows (instead of mingw's wrapper) makes sense, imo.

Hmmm, I think perhaps what we want to do is similar to what's being done here: https://github.com/maru/libmicrohttpd-http2/blob/316af7c06537e524c6ef02bc1d5f59b39af7d1d1/configure.ac#L1033-L1082

It's feature-based detection, and we'll have clarity between HAVE_C11_GMTIME_S, HAVE_W32_GMTIME_S, and HAVE_GMTIME_R.

Yeah, that's another way.
I wish this kind of basic functionality was standardized by now. This comes up so often.

If we still want this for 0.20, we need to make a decision here quickly though. I'm sorry for starting a bikeshed 😆

doesn't actually get printed in the gmtime_s success case.

Maybe I was just imaging this.. Have pushed up some new changes.

Tested on macOS:

checking for gmtime_r... yes

mingw-w64 cross compile:

checking for gmtime_r... no
checking for gmtime_s... yes

debian:

checking for gmtime_r... yes

MSVC ✅

Note one thing I have not tested with this implementation is how it operates if gmtime_r is not present, but C11-style gmtime_s is.

ACK a46484c

Post-merge ACK. Thanks for making this test-based, I think this is a nice improvement.

To make things clear about cross compiling on systems with pre-7.0 mingw-w64 headers, e.g., on bionic:

@hebasto the breakdown is: