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

Include <signal.h> in rgbgfx_test.cpp #1589

Merged
merged 1 commit into from
Dec 30, 2024
Merged

Include <signal.h> in rgbgfx_test.cpp #1589

merged 1 commit into from
Dec 30, 2024

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Dec 30, 2024

This is apparently necessary for BSD, and harmless for Linux. (It already gets pid_t from some other header, but POSIX has it in signal.h.)

@nunotexbsd This should mean that in 0.9.1 you can remove that patch file. :)

@Rangi42 Rangi42 added tests This affects the test suite rgbgfx This affects RGBGFX labels Dec 30, 2024
@Rangi42 Rangi42 added this to the 0.9.1 milestone Dec 30, 2024
@Rangi42 Rangi42 requested a review from ISSOtm December 30, 2024 16:03
@ISSOtm
Copy link
Member

ISSOtm commented Dec 30, 2024

This is surprising, since Linux + FreeBSD + OpenBSD's man pages document <spawn.h> as being sufficient for posix_spawn, which includes pid_t in its prototype. Further, Linux's man page says pid_t is defined in <sys/types.h>, and POSIX says that:

The <spawn.h> header shall define the mode_t and pid_t types as described in <sys/types.h>.

@nunotexbsd, do you know why that extra header is necessary?

@nunotexbsd
Copy link

nunotexbsd commented Dec 30, 2024

I was to do a pull request today

@nunotexbsd, do you know why that extra header is necessary?

No

@ISSOtm
Copy link
Member

ISSOtm commented Dec 30, 2024

I mean, why did you have to add that patch to the port?

@nunotexbsd
Copy link

I mean, why did you have to add that patch to the port?

build fails without it

@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 30, 2024

What is the exact message that it fails with? How did you choose to add that header on that line to fix it?

@nunotexbsd
Copy link

nunotexbsd commented Dec 30, 2024

What is the exact message that it fails with? How did you choose to add that header on that line to fix it?

build faild with missing symbols, missing header

I can send log laters when I get back to my PC

@nunotexbsd
Copy link

nunotexbsd commented Dec 30, 2024

I took info from the man:
https://man.freebsd.org/cgi/man.cgi?query=siginfo&apropos=0&sektion=0&manpath=FreeBSD+14.2-RELEASE+and+Ports&arch=default&format=html

FAILED: test/CMakeFiles/rgbgfx_test.dir/gfx/rgbgfx_test.cpp.o
/usr/bin/c++  -I/wrkdirs/usr/ports/devel/rgbds/work/rgbds/include -I/usr/local/include/libpng16 -O2 -pipe -fstack-protector-strong -fno-strict-aliasing -O2 -pipe -fstack-protector-strong -fno-strict-aliasing   -D
NDEBUG -std=gnu++20 -Wall -pedantic -Wno-gnu-zero-variadic-macro-arguments -MD -MT test/CMakeFiles/rgbgfx_test.dir/gfx/rgbgfx_test.cpp.o -MF test/CMakeFiles/rgbgfx_test.dir/gfx/rgbgfx_test.cpp.o.d -o test/CMakeFi
les/rgbgfx_test.dir/gfx/rgbgfx_test.cpp.o -c /wrkdirs/usr/ports/devel/rgbds/work/rgbds/test/gfx/rgbgfx_test.cpp
/wrkdirs/usr/ports/devel/rgbds/work/rgbds/test/gfx/rgbgfx_test.cpp:310:2: error: unknown type name 'siginfo_t'
  310 |         siginfo_t info;
      |         ^
/wrkdirs/usr/ports/devel/rgbds/work/rgbds/test/gfx/rgbgfx_test.cpp:313:29: error: use of undeclared identifier 'CLD_EXITED'
  313 |         } else if (info.si_code != CLD_EXITED) {
      |                                    ^
/wrkdirs/usr/ports/devel/rgbds/work/rgbds/test/gfx/rgbgfx_test.cpp:319:23: error: use of undeclared identifier 'CLD_DUMPED'
  319 |                     info.si_code == CLD_DUMPED ? " (core dumped)" : "",
      |                                     ^
3 errors generated.

Full log:
rgbds-0.9.0.log

@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 30, 2024

rgbgfx_test.cpp:310:2: error: unknown type name 'siginfo_t'

POSIX <sys/wait.h> marks siginfo_t as an "XSI extension".
POSIX <spawn.h> also says that "inclusion [...] may make visible symbols defined in the <sched.h>, <signal.h>, and <sys/types.h> headers", but does not guarantee it.
On the other hand, POSIX <signal.h> guarantees to define it.

rgbgfx_test.cpp:313:29: error: use of undeclared identifier 'CLD_EXITED'
rgbgfx_test.cpp:319:23: error: use of undeclared identifier 'CLD_DUMPED'

These are listed specifically in POSIX <signal.h>. Presumably that header is being made visible via one of our Linux headers, but not on BSD. So I think this PR is appropriate.

Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Oh, I see: the transitive exposition was made mandatory in POSIX.1-2024! So yeah, let's do this.

@ISSOtm ISSOtm merged commit 06daf2a into gbdev:master Dec 30, 2024
23 checks passed
@Rangi42 Rangi42 deleted the signal branch December 30, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rgbgfx This affects RGBGFX tests This affects the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants