-
Notifications
You must be signed in to change notification settings - Fork 81
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
Windows support #263
base: master
Are you sure you want to change the base?
Windows support #263
Conversation
38263ad
to
d93b00e
Compare
Regarding zlib, there might be some additional logic at https://github.com/DanielRuf/php-spx/pull/1/files that can be helpful. I didn't test this so far due to other more important things. |
d3b1a59
to
114469d
Compare
@DanielRuf Yes, I now have to add zlib dev package to the windows image. I'm looking for an example, without success so far. |
Maybe https://github.com/DanielRuf/spx-windows-build/actions/runs/10763553391/job/29845207008#step:2:54 I also found this: https://wiki.php.net/internals/windows/libs/apache22 https://github.com/madler/zlib/tree/develop/win32 Not sure if there is some more step needed to compile and load the zlib library. |
953727e
to
ac17287
Compare
I think these should be all relevant files. I will check later, because a quick search at https://github.com/search?q=repo%3Aphp%2Fphp-windows-builder+libraries&type=code makes me think, that we can simply drop the files in some Upon further inspection, this is used via |
@cmb69 before I am going in circles, where can we drop the files to let config.w32 find them with the least amount of changes and most portable setup ( |
You can download suitable zlib packages from https://downloads.php.net/~windows/php-sdk/deps/. Choose the toolset you want to build against (vs16 is for Visual Studio 2019, vs17 is for Visual Studio 2022), the arch (x64 or x86), and use the latest zlib (zlib-1.3.1-*.zip). Other builds may or may not work, depending on config.w32 and maybe other details). For CI, it is usually simpler to use the php/php-windows-builder or php/setup-php-sdk actions. The latter is the precursor of the former, and as such not as automated, but might be easier to understand. See https://github.com/php/pecl-database-dbase/blob/0029c2dad0927e96575ae509081a8f32f8ce11fb/.github/workflows/ci.yml#L44-L76 for a simple example. To also have zlib installed, just add |
@cmb69 would be https://github.com/DanielRuf/php-spx/blob/windows-build/.github/workflows/main.yml sufficient? |
src/spx_fmt.c
Outdated
#ifdef _WIN32 | ||
# pragma warning(push) | ||
# pragma warning(disable: 4129) | ||
#endif | ||
spx_output_stream_printf(output, "\e[%sm", row->cells[i].ansi_fmt); | ||
#ifdef _WIN32 | ||
#pragma warning(pop) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are sure that you want to disable this warning everywhere, you can add to config.w32:
ADD_FLAG("CFLAGS_SPX", "/wd4129");
and drop the #ifdef
s from the C files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I did for other warnings https://github.com/NoiseByNorthwest/php-spx/pull/263/files#diff-cfe793e01b76cf09eab98bc88e0b4d03c4524ce3dabff25010a3a7eae4d4277eR8
But for this one I'm not totally sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think \e
will work on Windows; maybe \x1b
would, but that requires Windows 10 or a console with ANSI color support. Unless the colors are important, consider to introduce a conditionally defined macro, and use this instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmb69 to be honest I haven't checked yet what is the actual support for ANSI sequences on windows. Given what you say I might disable color support (everything based on ANSI sequences) on windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could call php_win32_console_fileno_has_vt100()
to check for ANSI sequence support dynamically. This function is available only as of PHP 7.2, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it returns true
does it mean that it will work the same way as on unix-like systems (with \e[
as prefix) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, but \e
is not supported on Windows; you would need to use 0x1b
or something like that (ASCII value of ESC),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok 👍
src/php_spx.h
Outdated
#ifdef _WIN32 | ||
# pragma warning(push, 0) | ||
#endif | ||
#include "main/php.h" | ||
#ifdef _WIN32 | ||
# pragma warning(pop) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including (main/)php.h is not supposed to trigger any serious warnings; what happens in your case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See here https://github.com/NoiseByNorthwest/php-spx/actions/runs/10761275107/job/29840278337
For instance
C:\php-src\Zend\zend_config.w32.h(23): error C2220: the following warning is treated as an error (compiling source file ext\php-spx\src/spx_profiler.c)
C:\php-src\Zend\zend_config.w32.h(23): warning C4464: relative include path contains '..' (compiling source file ext\php-spx\src/spx_profiler.c)
C:\php-src\Zend\zend_portability.h(47): warning C4464: relative include path contains '..' (compiling source file ext\php-spx\src/spx_config.c)
C:\php-src\Zend\zend_portability.h(47): warning C4464: relative include path contains '..' (compiling source file ext\php-spx\src/spx_php.c)
C:\php-src\Zend\zend_portability.h(47): warning C4464: relative include path contains '..' (compiling source file ext\php-spx\src/spx_profiler.c)
C:\php-src\Zend\zend_portability.h(47): warning C4464: relative include path contains '..' (compiling source file ext\php-spx\src/spx_metric.c)
C:\php-src\Zend\zend_types.h(405): warning C4201: nonstandard extension used: nameless struct/union (compiling source file ext\php-spx\src/spx_profiler.c)
C:\php-src\Zend\zend_types.h(405): warning C4201: nonstandard extension used: nameless struct/union (compiling source file ext\php-spx\src/spx_metric.c)
C:\php-src\Zend\zend_types.h(405): warning C4201: nonstandard extension used: nameless struct/union (compiling source file ext\php-spx\src/spx_config.c)
C:\php-src\Zend\zend_types.h(405): warning C4201: nonstandard extension used: nameless struct/union (compiling source file ext\php-spx\src/spx_php.c)
C:\php-src\Zend\zend_alloc.h(27): warning C4464: relative include path contains '..' (compiling source file ext\php-spx\src/spx_metric.c)
C:\php-src\Zend\zend_alloc.h(27): warning C4464: relative include path contains '..' (compiling source file ext\php-spx\src/spx_config.c)
C:\php-src\Zend\zend_alloc.h(27): warning C4464: relative include path contains '..' (compiling source file ext\php-spx\src/spx_php.c)
C:\php-src\Zend\zend_alloc.h(27): warning C4464: relative include path contains '..' (compiling source file ext\php-spx\src/spx_profiler.c)
C:\php-src\Zend\zend_gc.h(156): warning C4242: '=': conversion from '__int64' to 'int', possible loss of data (compiling source file ext\php-spx\src/spx_config.c)
C:\php-src\Zend\zend_gc.h(156): warning C4242: '=': conversion from '__int64' to 'int', possible loss of data (compiling source file ext\php-spx\src/spx_metric.c)
C:\php-src\Zend\zend_gc.h(156): warning C4242: '=': conversion from '__int64' to 'int', possible loss of data (compiling source file ext\php-spx\src/spx_php.c)
C:\php-src\Zend\zend_gc.h(156): warning C4242: '=': conversion from '__int64' to 'int', possible loss of data (compiling source file ext\php-spx\src/spx_profiler.c)
C:\php-src\Zend\zend_hash.h(15[98](https://github.com/NoiseByNorthwest/php-spx/actions/runs/10761275107/job/29840278337#step:9:99)): warning C4127: conditional expression is constant (compiling source file ext\php-spx\src/spx_config.c)
C:\php-src\Zend\zend_hash.h(1598): warning C4127: conditional expression is constant (compiling source file ext\php-spx\src/spx_php.c)
C:\php-src\Zend\zend_hash.h(1598): warning C4127: conditional expression is constant (compiling source file ext\php-spx\src/spx_metric.c)
C:\php-src\Zend\zend_hash.h(1598): warning C4127: conditional expression is constant (compiling source file ext\php-spx\src/spx_profiler.c)
C:\php-src\Zend\zend_ast.h(374): warning C4242: 'function': conversion from 'uint32_t' to 'zend_ast_attr', possible loss of data (compiling source file ext\php-spx\src/spx_profiler.c)
C:\php-src\Zend\zend_ast.h(374): warning C4242: 'function': conversion from 'uint32_t' to 'zend_ast_attr', possible loss of data (compiling source file ext\php-spx\src/spx_config.c)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are from a quick glance, relatively harmless (level 3 and level 4). If you want to keep the /WX
flag, consider to add /W2
(that will disable all level 3 and level 4 warnings). Default for more recent PHP versions is /WX /W1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK 👍
zend_string * source_string, | ||
const | ||
#else | ||
zval * source_string, | ||
#endif | ||
char * filename | ||
#if PHP_API_VERSION >= 20210903 | ||
#if ZEND_MODULE_API_NO >= 20210903 | ||
, zend_compile_position position | ||
#endif | ||
TSRMLS_DC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need the TSRMLS_*
parameters? These are no-ops as of PHP 7.0.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes SPX still supports PHP 5.6
Possibly. Just try it out. :) |
cd0343a
to
8401264
Compare
Looks better now with the new action. I guess If I can help wit something there, let me know @NoiseByNorthwest. |
@DanielRuf Yes the zlib issue is solved, thanks to you and @cmb69. It is now up to me, the |
I may also drop some features for windows and consequently update the list here https://github.com/NoiseByNorthwest/php-spx/pull/263/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R68 |
8401264
to
f0bf2ef
Compare
df65b59
to
7370b8e
Compare
7370b8e
to
2abff9e
Compare
Initiated from the work of @DanielRuf here https://github.com/DanielRuf/spx-windows-build