-
Notifications
You must be signed in to change notification settings - Fork 182
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
Remove warning/errors about undefined functions #399
Conversation
see go-gl#359 features.h needs `_GNU_SOURCE` to be set to allow stdlib.h to declare the functions.
fixes #359 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems okay to me.
We had this in v3.2 but didn't propagate it forward to v3.3:
Lines 25 to 26 in a69d953
#cgo linux,!wayland CFLAGS: -D_GLFW_X11 -D_GNU_SOURCE | |
#cgo linux,wayland CFLAGS: -D_GLFW_WAYLAND -D_GNU_SOURCE |
(It was added there in #212 and 691ee1b.)
Thanks. Giving a chance to @Jacalz to also take a look before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I am however getting this warning on both X11 and Wayland compilations on Fedora Silverblue 39. Is there any way that we can avoid getting warnings about multiple definitions?
# github.com/go-gl/glfw/v3.3/glfw
In file included from ../../c_glfw_lin.go:10:
../../glfw/src/wl_window.c:29:9: warning: "_GNU_SOURCE" redefined
29 | #define _GNU_SOURCE
| ^~~~~~~~~~~
<command-line>: note: this is the location of the previous definition
Could it be that the file that previously was giving the warnings should have a #define _GNU_SOURCE
instead and these build changes should be removed?
EDIT: I tried my suggested fix and it does not seem to improve anything :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking more, I found we removed these from v3.3 in #356 (to fix #351). So I agree it seems this needs more investigation. Also see past discussion in glfw/glfw#2133 and glfw/glfw#2076.
I missed that removing this from v3.3 was done intentionally previously.
Hmm. I have one idea. Given that those warnings on Wayland now are errors after the linker changes in Fedora 40 (I've confirmed what @metal3d saw), how about we just enable GNU_SOURCE for Wayland builds? It's obviously just a short-term fix but it does improve things quite a bit. Anyone using Fedora 40 can then compile the binary (three errors become one warning) and everyone else gets one warning instead of three. Seems like a win until we manage to get all the warnings to disappear? |
I agree @Jacalz, this is critical on Fedora 40 and probably with others distributions that are up to date. Without this fix, it's impossible to build any Fyne application with Wayland support. A warning isn't problematic as it's only an already defined directive. IMHO this shouldn't be a problem, even if the warning is fixed, later, in the glfw project. |
Yeah, but given that there is no compilation problem on X11, I see not point in adding a new warning just for the sake of it. Would you mind keeping it enabled only for Wayland? |
FYI, I made the change to only set the directive to compile on Wayland. It is a very important fix that allows Fyne application to compile on Fedora 40. Even if it's temporary, this fix is highly necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review. I was away running a half-marathon during the weekend. This looks great. Thanks for the fix.
Fixes #359
features.h needs
_GNU_SOURCE
to be set to allow stdlib.h to declare the functions.