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

mlt_property.h: Replace include xlocale.h by locale.h #248

Closed
wants to merge 1 commit into from

Conversation

schnitzeltony
Copy link

xlocale.h was removed in glibc 2.26

I could test glibc case only. If it won't work for other environments, please consider this patch as ping.

xlocale.h was removed in glibc 2.26

Signed-off-by: Andreas Müller <schnitzeltony@googlemail.com>
@@ -31,7 +31,7 @@
#endif

#if defined(__GLIBC__) || defined(__APPLE__) || (__FreeBSD_version >= 900506)
Copy link
Member

Choose a reason for hiding this comment

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

This #if needs to be broken up if you are addressing a glibc change. Also, I think there should be a glibc version check as well with perhaps older versions still #include xlocale.h.

Copy link

Choose a reason for hiding this comment

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

#if defined(GLIBC) || defined(APPLE) || (__FreeBSD_version >= 900506)
+#if GLIBC_MINOR >= 26
+#include <locale.h>
+#else
#include <xlocale.h>
+#endif
#else
typedef char* locale_t;
#endif
is this ok?

@ddennedy
Copy link
Member

ddennedy commented Aug 8, 2017

What does "patch as ping" mean?

@ddennedy ddennedy added this to the v6.6.0 milestone Aug 8, 2017
@schnitzeltony
Copy link
Author

schnitzeltony commented Aug 8, 2017 via email

@ddennedy
Copy link
Member

ddennedy commented Aug 8, 2017

Travis is using Ubuntu 14.04 with glibc 2.19. It must also build on Debian 8, which is also v2.19.

If so: Would it be enough to split out glibc case only?

Yes, I believe so,

@plater
Copy link

plater commented Aug 25, 2017

mlt, shotcut and webvfx all have this issue in openSUSE:Factory which has recently updated to glibc-2.26 can I safely patch them to use locale.h until this issue is resolved in version updates?

@schnitzeltony
Copy link
Author

schnitzeltony commented Aug 25, 2017 via email

@plater
Copy link

plater commented Aug 25, 2017

I've made a patch:
--- src/framework/mlt_property.h.orig 2016-11-16 08:53:11.000000000 +0200
+++ src/framework/mlt_property.h 2017-08-25 14:37:05.144802219 +0200
@@ -31,7 +31,11 @@
#endif

#if defined(__GLIBC__) || defined(__APPLE__) || (__FreeBSD_version >= 900506)
+#if __GLIBC_MINOR__ >= 26 && !defined(__APPLE__)
+#include <locale.h>
+#else
#include <xlocale.h>
+#endif
#else
typedef char* locale_t;
#endif
It's on it's way to Factory, I'm sure FreeBSD uses a normal glibc but somebody pointed out that Apple have they're own.

@ddennedy
Copy link
Member

The last comment patch looks good to me, and I will apply it over the weekend if you do not want to bother to make a pull request. Thank you for your help.

@Hoshpak
Copy link

Hoshpak commented Aug 30, 2017

I believe something was lost here, possibly due to github formatting. I tried this patch on my system and it would still include xlocale.h and fail to compile. While looking for the cause, I discovered that GLIBC_MINOR was not defined anywhere however __GLIBC_MINOR__ was. After changing that, I was able to compile again. I think the same goes for APPLE versus __APPLE__ but don't have a system here to verify it.

@plater
Copy link

plater commented Aug 30, 2017

I never noticed that either, yes APPLE also lost it's underscores. Maybe this helps
https://build.opensuse.org/package/view_file/home:plater/libmlt/libmlt-nomorexlocale_h.patch?expand=1
or the download link
https://build.opensuse.org/source/home:plater/libmlt/libmlt-nomorexlocale_h.patch?rev=9356a09614d5e9080559752c24065dbe
I'm still new to github, I can't understand why you can't attach patches. Maybe it's better to add a .txt extension and attach it.

@plater
Copy link

plater commented Aug 30, 2017

Got it, I need a ` on each end

@ddennedy
Copy link
Member

I just pushed a commit for this underscore issue fbf6a51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants