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

fix: do not define feature test macros for BSD #126

Closed
wants to merge 1 commit into from

Conversation

cortexmancer
Copy link
Contributor

@cortexmancer cortexmancer commented Sep 27, 2024

When trying to compile nextvi for freebsd the following error happens:

sh cbuild.sh build
-> Entering step: "Build "nextvi" using "cc""
-> cc vi.c -o vi -O2 -pedantic -Wall -Wextra -Wno-implicit-fallthrough -Wno-missing-field-initializers -Wno-unused-parameter -Wno-unused-result -Wfatal-errors -std=c99 -D_POSIX_C_SOURCE=200809L  -D_BSD_SOURCE
vi.c:2006:14: fatal error: use of undeclared identifier 'SIGWINCH'
 2006 |                         sigaction(SIGWINCH, &sa, NULL))
      |                                   ^
1 error generated.
-> Failed during step: "Build "nextvi" using "cc"

Browsing over web, I could found that can be fixed with the addition of __BSD_VISIBLE flag, but I also found alot of interesant info about of the subject and started to dig a lot more.
Take for example the following issue in the repo: nsf/termbox#110

rofl0r stated:

__BSD_VISIBLE is an internal macro, that's not the right fix. you need to use the right feature test macro http://man7.org/linux/man-pages/man7/feature_test_macros.7.html

Now the following thread on freebsd mail list:

https://lists.freebsd.org/pipermail/freebsd-current/2018-May/069355.html

Stefan Hagen awnser himself:

Answering myself. I had -D_XOPEN_SOURCE=700 defined which essentially blocks
access to SIGWINCH. Not defining -D_XOPEN_SOURCE on FreeBSD fixed my issue.

and later Garrett Wollman:

The right way is to not request strict adherence to a standard that
doesn't include that signal. Do not define any of these macros and
you will get SIGWINCH defined.

Well, the subject itself is about D_XOPEN_SOURCE but it is the same for D_POSIX_C_SOURCE as I finally can state with this really nice thread:

https://lists.freebsd.org/pipermail/freebsd-standards/2004-March/000474.html

Q3: What is the right and portable set of flags to use?
There is none. For FreeBSD, you probably need a null set of flags.
For Linux, this is the wrong list to ask on :-).

So in BSD system, we do not require D_POSIX_C_SOURCE.
D_BSD_SOURCE in thruth also does nothing for the system context:

Q1: Why does Linux require _BSD_SOURCE but FreeBSD requires __BSD_VISIBLE?
Linux apparently requires _BSD_SOURCE to give BSD extensions. This is
a bug in Linux. The BSD extensions are better of course :-), and feature
tests macros should generally restrict, not enable extensions. I think
Linux does this because some BSD extensions are incompatible.
FreeBSD doesn't require __BSD_VISIBLE. On the contrary, __BSD_VISIBLE is
an implementation detail, and setting it in applications gives undefined
behaviour.

My changes then, add D_POSIX_C_SOURCE as an requirement for linux and Darwin, and remove an BSD exclusive definition.

As Darwin and BSD are related, maybe this could be applied to it as well, but since I cant test it so my decision was to let it there.

I dont know if this will look reasonable for you, but I hope it can be a nice addition to the build process.

Thank you for the time.

@Un1q32
Copy link
Contributor

Un1q32 commented Sep 27, 2024

The case statement for linux has a trailing space

@cortexmancer
Copy link
Contributor Author

cortexmancer commented Sep 27, 2024

The case statement for linux has a trailing space

You mean the space in the end of line?
I did an ammend, as I dont see reason for a whole commit for that.

@kyx0r
Copy link
Owner

kyx0r commented Sep 27, 2024

Honestly, this problem can be avoided on every platform by just defining DEFAULT_SOURCE or not defining the *_SOURCE at all.
It's there to prevent people from using non portable gnu extended version of functions. Hope is SIGWINCH will be added to new posix because it's so widespread.

@kyx0r
Copy link
Owner

kyx0r commented Sep 27, 2024

Patch applied, thanks!

@kyx0r kyx0r closed this Sep 27, 2024
@cortexmancer
Copy link
Contributor Author

cortexmancer commented Sep 27, 2024

Honestly, this problem can be avoided on every platform by just defining DEFAULT_SOURCE or not defining the *_SOURCE at all. It's there to prevent people from using non portable gnu extended version of functions. Hope is SIGWINCH will be added to new posix because it's so widespread.

Yeah, I just changed D_POSIX_C_SOURCE to D_DEFAULT_SOURCE and it really works.

In the mean time you accepted this PR, but dont you think to just set DEFAULT_SOURCE and remove the OS specifics would be a better and minimal addition?

@cortexmancer cortexmancer deleted the fix/bsd-sigwinch branch September 27, 2024 17:26
@kyx0r
Copy link
Owner

kyx0r commented Sep 27, 2024

Honestly, this problem can be avoided on every platform by just defining DEFAULT_SOURCE or not defining the *_SOURCE at all. It's there to prevent people from using non portable gnu extended version of functions. Hope is SIGWINCH will be added to new posix because it's so widespread.

Yeah, I just changed D_POSIX_C_SOURCE to D_DEFAULT_SOURCE and it really works.

In the mean time you accepted this PR, but dont you think to just set DEFAULT_SOURCE and remove the OS specifics would be a better and minimal addition?

No, because I want the compiler to not compile if non portable function gets used. As a workaround, I've added a default case with DEFAULT_SOURCE for a platform that has not been targeted. This should be ok.

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