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

Remove warning/errors about undefined functions #399

Merged
merged 2 commits into from
May 6, 2024

Conversation

metal3d
Copy link
Contributor

@metal3d metal3d commented Apr 23, 2024

Fixes #359

features.h needs _GNU_SOURCE to be set to allow stdlib.h to declare the functions.

see go-gl#359

features.h needs `_GNU_SOURCE` to be set to allow stdlib.h to declare
the functions.
@Jacalz Jacalz self-requested a review April 23, 2024 22:16
@metal3d
Copy link
Contributor Author

metal3d commented Apr 23, 2024

fixes #359

dmitshur
dmitshur previously approved these changes Apr 23, 2024
Copy link
Member

@dmitshur dmitshur left a 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:

glfw/v3.2/glfw/build.go

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.

Copy link
Collaborator

@Jacalz Jacalz left a 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 :(

@dmitshur dmitshur self-requested a review April 24, 2024 18:15
Copy link
Member

@dmitshur dmitshur left a 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.

@dmitshur dmitshur dismissed their stale review April 24, 2024 18:18

I missed that removing this from v3.3 was done intentionally previously.

@Jacalz
Copy link
Collaborator

Jacalz commented Apr 25, 2024

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?

@metal3d
Copy link
Contributor Author

metal3d commented Apr 26, 2024

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.

@Jacalz
Copy link
Collaborator

Jacalz commented Apr 26, 2024

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?

@metal3d metal3d requested a review from Jacalz May 3, 2024 07:29
@metal3d
Copy link
Contributor Author

metal3d commented May 6, 2024

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.

Copy link
Collaborator

@Jacalz Jacalz left a 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.

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.

v3.3/glfw: Warnings when building using -tags wayland
3 participants