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

Update frankenphp.c for compilation on FreeBSD #1058

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

rwv37
Copy link
Contributor

@rwv37 rwv37 commented Oct 1, 2024

Compilation would fail on FreeBSD due to a missing header file (pthread.h). This commit adds a #include for it.

Compilation would fail on FreeBSD due to a missing header file (``pthread.h``). This commit adds a ``#include`` for it.
@dunglas dunglas merged commit 1e20f65 into dunglas:main Oct 2, 2024
26 of 32 checks passed
@dunglas
Copy link
Owner

dunglas commented Oct 2, 2024

Thanks!

@rwv37
Copy link
Contributor Author

rwv37 commented Oct 2, 2024

Hmmm... I'm sorry, but I just realized something potentially problematic. I seriously doubt that it actually is problematic, but I feel obligated to note it:

I put the line inside of the same #ifdef block that the existing #include <pthread_np.h> was in. That's fine for my machine, which is running FreeBSD, but the block also applies to OpenBSD.

Looking at OpenBSD's source for phtread_np.h and pthread.h, it seems to me like this change is almost certainly not needed for modern OpenBSD, as their phtread_np.h itself has a #include for pthread.h. On the other hand it shouldn't hurt, as pthread.h has a standard #ifndef/#define guard in place.

Plus that #include <pthread.h> has been there for five years, so I guess this change might help if for some reason someone wants to run FrankenPHP on some ancient, unsupported version of OpenBSD ¯\_(ツ)_/¯

But strictly speaking, I don't know if it's OK or not. I haven't tested it on OpenBSD (which I've never installed anywhere, and which as far as I can remember I've never knowingly used at all ever in my life).

@travisghansen
Copy link

I am trying to package this for arch linux and ran into this same issue, can we just include it generally?

@dunglas
Copy link
Owner

dunglas commented Oct 16, 2024

@travisghansen yes, I guess it won't hurt. Would you mind to open a PR?

@travisghansen
Copy link

@dunglas sorry for the delay, yes I can but I may just use the prebuilt binaries (although that is less than ideal). I was getting errors about hitting max stack size (iirc). Are there specific build args I should be using to match what you do?

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