libfaketime, which is used in our deterministic build environment, can't handle the library timestamps. The attached patch disables therefore disables them.

Comment on attachment 8388445 [details] [diff] [review] disabling library timestamping Review of attachment 8388445 [details] [diff] [review]: ----------------------------------------------------------------- This is the wrong way to do it if we really want to do it. At the very least, the change should be in the corresponding Makefile. But I think we should just go with bug 838566. Wan Teh, what do you think?

Comment on attachment 8388445 [details] [diff] [review] disabling library timestamping Review of attachment 8388445 [details] [diff] [review]: ----------------------------------------------------------------- It seems that the SH_DATE command in these makefiles should also be problematic for your deterministic builds: http://mxr.mozilla.org/nspr/search?string=SH_DATE Does libfaketime handle the 'date' command properly?

(In reply to Wan-Teh Chang from comment #2) > Does libfaketime handle the 'date' command properly? Yes. We only have problems with the high-precision timestamps. Thus, if we could have an option/way to disable them (be it only in the context of deterministic builds) that would be pretty helpful.

Georg: Thanks for the reply. So I can also fix this bug by changing the gettimeofday() call in nspr/config/now.c to a time() call, right? http://pubs.opengroup.org/onlinepubs/7908799/xsh/time.html

(In reply to Wan-Teh Chang from comment #4) > Georg: Thanks for the reply. So I can also fix this bug by changing > the gettimeofday() call in nspr/config/now.c to a time() call, right? At least the Linux part is fine then, yes. Not sure what to do about GetSystemTimeAsFileTime() as this bit us as well in the past (we are cross-compiling for Windows on Linux using mingw-w64) and libfaketime is not handling it either.

Georg: good. Since time() is a Standard C Library function, I can use time() on all platforms. nspr/config/now.c does not need to output a high-precision timestamp. A precision of one second is enough. Note: Mike Hommey has convinced me that eventually I should get rid of the obsolete code that uses now.c. But I'll create a patch first to unblock you.

Comment on attachment 8390796 [details] [diff] [review] Call the Standard C Library function time() Review of attachment 8390796 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/now.c @@ -38,2 @@ > #endif > - now = ((1000000LL) * tv.tv_sec) + (long long)tv.tv_usec; tv.tv_sec has the type time_t: http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/time.h.html So we can replace tv.tv_sec with the "time_t sec" variable in the new code, and drop the tv.tv_usec term in this expression.

Comment on attachment 8390796 [details] [diff] [review] Call the Standard C Library function time() Review of attachment 8390796 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, thanks!

I rearranged the code to avoid nested ifdefs and checked in the patch: https://hg.mozilla.org/projects/nspr/rev/312cd51c0dd9

Comment on attachment 8390796 [details] [diff] [review] Call the Standard C Library function time() Review of attachment 8390796 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/now.c @@ +26,5 @@ > #endif > + time_t sec; > + > + sec = time(NULL); > + now = (1000000LL) * sec; I forgot to note that the reason I rearranged the ifdefs was that I need to use the i64 suffix for this 64-bit integer literal with older versions of Visual C++, and I thought the nested ifdefs would be excessive. When we don't need to support Visual C++ versions that don't have the long long type, we can remove the _WIN32 code in this file. Perhaps we can already do that now. I just don't have time to research this.

Looking at the old code, I saw an __GNUC__ ifdef inside the _WIN32 code. So I changed our _WIN32 test to a _MSC_VER test. https://hg.mozilla.org/projects/nspr/rev/8f05ea71fdda

Comment on attachment 8391601 [details] [diff] [review] Test _MSC_VER instead of _WIN32 for Visual C++ only language constructs Review of attachment 8391601 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, too. Thanks!