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

[rglfw] fix: suppress warning: "_GNU_SOURCE" redefined #4720

Conversation

sleeptightAnsiC
Copy link
Contributor

@sleeptightAnsiC sleeptightAnsiC commented Jan 23, 2025

When compiling rglfw.c with recent versions of GCC or Clang following warning appears:

$ git clone https://github.com/raysan5/raylib.git
$ cd raylib/src
$ make rglfw.o CC=gcc
gcc  -c rglfw.c -Wall -D_GNU_SOURCE -DPLATFORM_DESKTOP_GLFW -DGRAPHICS_API_OPENGL_33 -Wno-missing-braces -Werror=pointer-arith -fno-strict-aliasing -std=c99 -fPIC -O1 -Werror=implicit-function-declaration -D_GLFW_X11  -I.  -Iexternal/glfw/include
In file included from rglfw.c:99:
external/glfw/src/posix_poll.c:27:9: warning: "_GNU_SOURCE" redefined
   27 | #define _GNU_SOURCE
      |         ^~~~~~~~~~~
<command-line>: note: this is the location of the previous definition

This PR suppresses said warning by checking, if _GNU_SOURCE has NOT been defined, and only then defining it. It does not change any behavior, just suppresses the warning.

Raylib already handles similar cases elsewhere in it's source tree (e.g. in rcore.c) so this is nothing new.

When compiling rglfw.c with GCC or Clang following warning appears:

In file included from rglfw.c:99:
external/glfw/src/posix_poll.c:27:9: warning: "_GNU_SOURCE" redefined
   27 | #define _GNU_SOURCE
      |         ^~~~~~~~~~~

This commit suppresses said warning by checking,
if _GNU_SOURCE has NOT been defined, and only then defining it.

Since said macro has been always defined in posix_poll and wl_window,
this commit does not change any behavior at all (it just suppresses the
warning, that's it).
@raysan5
Copy link
Owner

raysan5 commented Jan 24, 2025

@sleeptightAnsiC I'm afraid this issue belongs to an external library and should be addressed in GLFW repo instead of raylib to avoid being rewritten when updating to next GLFW version.

@raysan5 raysan5 closed this Jan 24, 2025
@sleeptightAnsiC
Copy link
Contributor Author

Hi @raysan5 ,

This issue does not belong to GLFW upstream because Raylib is the one causing it. Some of our build systems define _GNU_SOURCE at command line thus triggering this. It simply does not occur on their side and they rejected similar contributions (see glfw/glfw#2133 and glfw/glfw#2076 )

Perhaps, this can be fixed in our build systems? I've noticed only two affected: src/Makefile and build.zig. For comparison, Cmake does not define it and works just fine. So maybe -D_GNU_SOURCE is a relic of the past, obsolete in current versions of Raylib? I've noticed that most sources (re)define it anyway when needed.

Hope you're doing well.
Best regards.

@raysan5
Copy link
Owner

raysan5 commented Jan 24, 2025

@sleeptightAnsiC Still, the change applies to GLFW and it will be overriden on update. Afair, @Peter0x44 added? that flag on the past for some specific use-case, maybe he can provide more details and if it is still required.

Personally, I'm fine removing it completely from raylib building.

@Peter0x44
Copy link
Contributor

I can't remember why I needed it, I think it was for accessing clock functions from C99.
C11 has them as standard so it's not needed there.

The warning is preferable to patching glfw, imo, I hope there's a better solution.
Ideally feature test macros go on the command line if possible.

@sleeptightAnsiC
Copy link
Contributor Author

sleeptightAnsiC commented Jan 25, 2025

It's a bit strange that Makefile and zig builds can trigger this but Cmake does not. Is Cmake just not defining it or suppressing with something like -Wno-builtin-macro-redefined ? @Peter0x44

It would be nice if all build systems were consistent here, either all defining _GNU_SOURCE at command line or all not doing it. Current difference could cause some cryptic issues in the future where functions depending on specific glibc behavior would become buggy in non-glibc environments.

Anyway, I agree this PR was wrong. I'll see later if maybe resolving this in GLFW is possible, since it's still a bit strange that posix_poll.c defines said macro but then uses several build branches for accessing correct variant of poll.

@Peter0x44
Copy link
Contributor

@sleeptightAnsiC I think I investigated this before, it's because cmake uses -std=gnu99 or so by default, which defines _GNU_SOURCE
Setting CMAKE_C_EXTENSIONS to off should make it fail.
I haven't tested this but will do so as soon as I can.

And suppressing the warning on the command line is not a good idea imo.

@sleeptightAnsiC
Copy link
Contributor Author

And suppressing the warning on the command line is not a good idea imo.

No no, I'm not saying it should be suppressed, just was wondering if it was.

it's because cmake uses -std=gnu99 or so by default, which defines _GNU_SOURCE

Well, Makefile and build.zig are also using gnu99, but they redefine _GNU_SOURCE unlike Cmake.

@Peter0x44
Copy link
Contributor

ifeq ($(TARGET_PLATFORM),$(filter $(TARGET_PLATFORM),PLATFORM_WEB PLATFORM_WEB_RGFW))

The makefile only uses it for web and rgfw web
So that's not exactly true.

I did not check build.zig.

@sleeptightAnsiC
Copy link
Contributor Author

Ahh, I see, sorry.

zig seems to be using -std=gnu99 unconditionally and also defining -D_GNU_SOURCE at the same time.

raylib/build.zig

Lines 81 to 88 in 49d37b0

try raylib_flags_arr.appendSlice(&[_][]const u8{
"-std=gnu99",
"-D_GNU_SOURCE",
"-DGL_SILENCE_DEPRECATION=199309L",
"-fno-sanitize=undefined", // https://github.com/raysan5/raylib/issues/3674
});

@Peter0x44
Copy link
Contributor

I think this requires a bit more research to figure how to properly solve and also isn't particularly urgent.
Do you mind opening an issue for this to discuss?

I can also do it if needed.

(And slight apology for my comments on our last interaction, figured it was worth a mention)
I'll be more attentive from now.

@sleeptightAnsiC
Copy link
Contributor Author

Do you mind opening an issue for this to discuss?

sure, I'll open it in a moment

(And slight apology for my comments on our last interaction, figured it was worth a mention)
I'll be more attentive from now.

No worries. I tried push my point of view really hard. For this I also apologize.

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.

3 participants