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 checks for gmtime_r availability. #126

Merged
merged 1 commit into from
Jan 29, 2021
Merged

Fix checks for gmtime_r availability. #126

merged 1 commit into from
Jan 29, 2021

Conversation

ericonr
Copy link
Contributor

@ericonr ericonr commented Jan 27, 2021

Actually check for the function in configure.ac, which is the only way
of definitively testing for it. This does not add such detection to any
other builds, such as the CMake one.

Add a warning for cases where gmtime() is used.

Also fix Http_Cookies where gmtime() was still being used.

Copy link
Member

@JeromeMartinez JeromeMartinez left a comment

Choose a reason for hiding this comment

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

Thank you for your PR, it helps to improve MediaInfo.
Some small change requests.
For CMake, seems that there is CheckFunctionExists and it would be nice to have for distros using CMake and without the #if I have put, but it is fine without it (we'll check).

For your question in the void-linux comment:

Do you know of any platforms that don't have it, besides Windows? (For which you already have an ifdef block).

MediaInfo is pretty old and used to be on platforms without them, but I don't remember which one.
Anyway, I prefer to keep maximum compatibility with strict (without non standard extensions) C/C++ by default (i there is no defined precompiler stuff) as much as possible.

Project/GNU/Library/configure.ac Outdated Show resolved Hide resolved
@@ -1312,14 +1312,15 @@ Ztring& Ztring::Date_From_Seconds_1970 (const int32s Value)
Ztring& Ztring::Date_From_Seconds_1970 (const int64s Value)
{
time_t Time=(time_t)Value;
#if _POSIX_C_SOURCE >= 1 || _XOPEN_SOURCE || _BSD_SOURCE || _SVID_SOURCE || _POSIX_SOURCE
#if defined(HAVE_GMTIME_R)
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the other #if (no change for people using CMake and so on)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other if works on GLIBC by accident, it isn't correct. Someone on musl and using CMake will have the same issue we did (unless we add the function check to CMake, which is probably best).

Copy link
Member

Choose a reason for hiding this comment

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

OK, maybe less problematic for everyone if we go directly to the step with CMake implementation.
@g-maxime if not long for you please hint about CMake changes to do (I found the WebKit corresponding test, maybe really easy to add in CMake, and @ericonr note the even a modern project like WebKit checks the presence of such function ;-) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have added the CMake and the autoconf checks! Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't seen your second response, and it appears WebKit has a function of their own for adding function definitions. I hope my implementation is enough.

Source/ZenLib/Ztring.cpp Show resolved Hide resolved
Source/ZenLib/Ztring.cpp Outdated Show resolved Hide resolved
Actually check for the functions in configure.ac and CMakeLists.txt,
which is the only way of definitively testing for them, since feature
test macros such as _POSIX_C_SOURCE are supposed to be used to tell
system headers what functions should be exposed, and the application
shouldn't check their values.

Add a warning for cases where gmtime() or localtime() are used.

Also fix Http_Cookies where gmtime() was still being used.
@JeromeMartinez JeromeMartinez merged commit 71c33ea into MediaArea:master Jan 29, 2021
@JeromeMartinez
Copy link
Member

Thanks.
@g-maxime please an intermediate release.

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.

None yet

2 participants