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

[feat] Code change for compile on windows. Needs to modify php-src. #1286

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TenHian
Copy link

@TenHian TenHian commented Dec 22, 2024

No description provided.

Copy link
Owner

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Thank you very much for moving Windows support forward!

frankenphp.c Outdated
Comment on lines 31 to 33
#elif defined(__MINGW64__)
#include <pthread.h>
#endif
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
#elif defined(__MINGW64__)
#include <pthread.h>
#endif

pthread.h is always included (line 20)

frankenphp.go Outdated
Comment on lines 18 to 21
// #cgo linux CFLAGS: -Wall -Werror
// #cgo darwin CFLAGS: -Wall -Werror
// #cgo openbsd CFLAGS: -Wall -Werror
// #cgo freebsd CFLAGS: -Wall -Werror
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// #cgo linux CFLAGS: -Wall -Werror
// #cgo darwin CFLAGS: -Wall -Werror
// #cgo openbsd CFLAGS: -Wall -Werror
// #cgo freebsd CFLAGS: -Wall -Werror
// #cgo unix CFLAGS: -Wall -Werror

Should work

@TenHian
Copy link
Author

TenHian commented Dec 24, 2024

Thanks for your comments. I've take them and added the compilation documentation.

@TenHian
Copy link
Author

TenHian commented Jan 7, 2025

@dunglas Is it still worthwhile to carry on with this route?

Copy link
Owner

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Yes, it looks like a good step forward Windows support to me.


if ($content !== $new_content) {
file_put_contents($file_path, $new_content);
echo "$file_path 中的 __forceinline 已替换为 inline\n";
Copy link
Owner

Choose a reason for hiding this comment

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

Where is this file coming from? We should add the proper copyright and license header. We should also translate it in English.

Also, can't this patch be upstreamed?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you.

This php script was written by myself to modify part of the code of php-8.3.0. So that the compiled .dll can be linked by the mingw-w64 linker. I should have forgotten to change it to English in this line. Slip-up.
echo "\"__forceinline\" in $file_path has been replace by \"inline\"\n";
I'll correct it.

About copyright. I read php license again and noticed that "Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer." Honestly, I'm not sure if the act of uploading a public repo of the script I used to modify the php source code counts as Redistribution. Maybe I should ask someone else.

About this patch could or not be upstreamed. I could sort this patch out and submit a pull request to php-src. but the patch is only meant to be compatible with frankenphp, and I'm not sure if it would be adopted. But I can give it a try.

Copy link
Owner

Choose a reason for hiding this comment

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

Forget what I said about the copyright, as you're the original author of this script, it's fine!

Copy link
Owner

Choose a reason for hiding this comment

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

That would be cool to upstream the patch. I don't see what is FrankenPHP-specific in it, it would be useful for all users of the embed SAPI on Windows, right?

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.

2 participants