refactor: windows build #12281

Lewiscowles1986 · 2023-09-23T08:05:39Z

As promised, this set of changes (builds on #12280 and #12264) pulls GitHub actions specific scripts outside of GitHub actions

The purpose is so that users are able to know they can use these outside of GitHub actions, and encourage deeper knowledge of windows build process.

It's very spartan. Please let me know what you would like to change or make clearer.

test.bat - named variables for capturing drive and script test_task.bat - using named variables, more readable psql & mysql, safer commands This is intended to lead to a future refactor, in which these scripts are removed from a GitHub actions specific folder

…pecific folder

Girgias · 2023-09-23T18:51:18Z

Windows CI is failing currently

Lewiscowles1986 · 2023-09-24T07:57:04Z

I can read thanks @Girgias; I think it's me trying to be too much of a smart-ass when refactoring, but I'll get on-top of it

Lewiscowles1986 · 2023-09-24T13:10:18Z

Fixed it @Girgias skipped setx, and went back to set; just really wanted %~dp0 and %~d0 to mean one consistent named thing in these scripts (it might mean something else in other scripts), but in these it's to deal with directory hopping and still wanting these scripts available.

Lewiscowles1986 · 2023-09-24T13:26:50Z

scripts/windows/cleanup.bat

+@echo off
+echo "Stopping Firebird Server..."
+sc query FirebirdServerTestInstance >nul 2>&1
+if errorlevel 1 (
+    echo "FirebirdServerTestInstance Service does not exist. Nothing to do."
+) else (
+	sc stop FirebirdServerTestInstance
+	sc delete FirebirdServerTestInstance
+	echo "Stopped FirebirdServerTestInstance"
+)
+
+echo "Stopping SNMPTrap..."
+sc query SNMPTrap >nul 2>&1
+if errorlevel 1 (
+    echo "SNMPTrap Service does not exist. Nothing to do."
+) else (
+	sc stop SNMPTrap
+	sc delete SNMPTrap
+	echo "Stopped SNMPTrap"
+)
+
+if EXIST C:\tests_tmp rmdir /s /q C:\tests_tmp
+if EXIST %PHP_BUILD_DIR%\test_file_cache rmdir /s /q %PHP_BUILD_DIR%\test_file_cache


I run a separate version of this locally. This is like the <10th iteration, but I want to get folks thinking on it. The tests crowd people's machines with lots of (I think frankly trash); but I can also see to make this fully flexible, folks really should have the choice to just turn off a service.

How then to ensure the services names are known globally rather than shared by convention like this?

iluuu1994 · 2023-09-25T13:45:16Z

.github/actions/setup-windows/action.yml

@@ -7,6 +7,8 @@ runs:
      run: |
        choco install mysql -y --no-progress --params="/port:3306"
        mysql.exe --port=3306 --user=root --password="" -e "ALTER USER 'root'@'localhost' IDENTIFIED BY 'Password12!'; FLUSH PRIVILEGES;"
+        mysql.exe --port=3306 --user=root --password="Password12!" -e "DROP DATABASE IF EXISTS test;"


Why? All jobs start from a clean slate.

Then this will cost nothing. 😄

I Get it, this script and the approach to maintaining them; are that the environment it happens to work in today, is great and will always be there and supported.

I Do not believe this to be the case.

Can you find me a case where this one-liner command will cause problems?

I Can tell you where problems will happen for anyone not using GitHub actions if they attempt to run a script more than once without a complete clean-room environment, which PHP core does not provide.

A Bit of background on these. If we did a modular PHP build, the commands here would be moved to batch files, folks with chocolatey (listed in the readme) could run. Not only do I think this would be more readable. It would help people to know how to build a box for building Windows. I think it's 1-2 iterations away, which is why I wanted to centralise this setup code for MySQL and PGSQL here (Unknown why MsSQL doesn't need any setup)

iluuu1994 · 2023-09-25T13:50:00Z

.github/workflows/nightly.yml

      - name: Test
-        run: .github/scripts/windows/test.bat
+        run: |
+          call ./scripts/windows/env.bat


Can't we just use default-values for the env vars?

you are using default values for env vars, in all but two cases. This is a limitation of GitHub actions more than windows.

iluuu1994 · 2023-09-25T13:50:39Z

scripts/windows/env.bat

@@ -0,0 +1,22 @@
+@echo off
+
+set WIN_SCRIPTS_DIR=%~dp0


This doesn't need to be configurable

No, but it is shared, so to make the batch files more readable.

iluuu1994 · 2023-09-25T13:53:20Z

scripts/windows/env.bat

+    set PARALLEL= -j2
+    set OPCACHE=1
+
+    set CI=True


If the scripts are intended to be run locally, we should remove the guard instead.

In part of your review, you acknowledge these scripts may not run in github actions. In other places you do not.

In any case these scripts are replacing lots of manual commands, so I think this shows that we are running CI (unattended) scripts locally, rather than writing a specific purpose local script.

I Have every desire to see if I can help folks not use this for CI. Right now GitHub actions, and my own computer, are the only places it is used.

iluuu1994 · 2023-09-25T13:54:03Z

scripts/windows/test_task.bat

    echo for CI only
    exit /b 3
 )

+echo WIN_SCRIPTS_DIR=%WIN_SCRIPTS_DIR%
+echo SCRIPT_DRIVE=%SCRIPT_DRIVE%


These echos serve no purpose, please remove them.

They actually serve the purpose of knowing the values have been passed. I'm happy to comment them, but they don't cost so much. Is this myopic attention to detail really necessary? It really didn't show in the scripts as they were when I first started reading them.

Mixed styles of coding.

Commented values (you're in Git, once something lands, comments can be removed).

Setup and initialisation of tests coupled to running tests.

Hardcoded to Github actions.

iluuu1994 · 2023-09-25T13:54:13Z

scripts/windows/test_task.bat

 if %errorlevel% neq 0 exit /b 3

 rem setup PostgreSQL related exts
 set PGUSER=postgres
 set PGPASSWORD=Password12!
 rem set PGSQL_TEST_CONNSTR=host=127.0.0.1 dbname=test port=5432 user=postgres password=Password12!
+git checkout "./ext/pgsql/tests/config.inc"


To reset the file to it's checked out state. This was explicitly written in #12179, which we interacted on. Like the drop database if not exists, this is so that people outside of GitHub actions can safely use this script without a clean-room environment, which nobody is providing. It will cost nothing in GitHub actions.

iluuu1994 · 2023-09-25T14:05:42Z

scripts/windows/README.md

+
+## build.bat
+
+The purpose of this script is to compile PHP, after setting up environment. It is intended to be run via command-line `cmd` under windows as an administrator.


I'm not sure build.bat is a good starting point for local builds. It contains hard-coded flags to configure.bat for CI. Usually a build of PHP doesn't require all bundled extensions.

I Agree, but this is what you have available. If you have an example of windows builds with optional extensions building, I'd enjoy your contribution or making the contribution later. This is exactly what the purpose of this readme is. How to use, what it does, and I believe there is a part of it about welcoming improvements.

Please also see #12281 (comment) as making dependencies optional would also affect cleanup

iluuu1994 · 2023-09-25T14:07:18Z

scripts/windows/README.md

+## env.bat
+
+The purpose of this script is simply to setup environment. It is intended to be run via command-line `cmd` under windows as an administrator.
+You can adjust values in this script to:


We shouldn't encourage editing committed files. Why can't these values be set using actual env variables?

The CI edits committed files today. Did you not know this? Did nobody tell you? Is there a plan to first change that? I can wait.

CI is never going to create new commits based on the edited files, but users developing/compiling locally might accidentally include edits to build files in unrelated commits. An env var (and/or command line arg?) would help to mitigate this problem.

This isn't creating new commits @DaveRandom the script has always edited config.inc (maybe other files). If that edit (echo "details" >> file) is run twice, outside of CI, which checks out a fresh copy of the file, then multiple lines are edited into that file. The git reset and editing the env.bat are just ways for folks to run the build with other settings. Technically more file-based edits are needed to build different PHP versions if you go back too far (even 8.2, 8.1 from 8.3) then different extensions are exercised.

Lewiscowles1986 · 2023-09-25T17:31:03Z

scripts/windows/test_task.bat

@@ -89,16 +89,16 @@ rem work-around for failing to dl(mysqli) with OPcache (https://github.com/php/p
 if "%OPCACHE%" equ "1" set OPCACHE_OPTS=%OPCACHE_OPTS% -d extension=mysqli

 rem prepare for enchant
-mkdir %~d0\usr\local\lib\enchant-2
+if NOT EXIST %SCRIPT_DRIVE%\usr\local\lib\enchant-2 mkdir %SCRIPT_DRIVE%\usr\local\lib\enchant-2


I'd like to call out that these also would not affect a clean-room environment, such as GitHub actions; and are not that readable to most competent engineers, because unlike Microsoft developers; in 30 years (probably took a week of frustration), posix devs created short-syntax mkdir -p.

Lewiscowles1986 · 2023-09-25T17:32:05Z

scripts/windows/test_task.bat

@@ -115,6 +115,7 @@ hMailServer.exe /verysilent
 cd %APPVEYOR_BUILD_FOLDER%
 %PHP_BUILD_DIR%\php.exe -dextension_dir=%PHP_BUILD_DIR% -dextension=com_dotnet appveyor\setup_hmailserver.php

+if EXIST %PHP_BUILD_DIR%\test_file_cache rmdir /s /q %PHP_BUILD_DIR%\test_file_cache


In this case instead of mkdir -p (ensure exists); I opted to clean the folder to ensure leftover state doesn't affect anything, or confuse anyone. I am unsure if this is correct.

Lewiscowles1986 · 2023-09-25T17:32:40Z

scripts/windows/test_task.bat

@@ -52,7 +52,7 @@ set PDOTEST_DSN=odbc:%ODBC_TEST_DSN%

 rem setup Firebird related exts
 curl -sLo Firebird.zip https://github.com/FirebirdSQL/firebird/releases/download/v3.0.9/Firebird-3.0.9.33560-0_x64.zip
-7z x -oC:\Firebird Firebird.zip
+7z x -y -oC:\Firebird Firebird.zip


This would not be needed for a clean-room environment, as it should not be possible to overwrite files that did not exist.

Lewiscowles1986 · 2023-09-25T17:33:19Z

scripts/windows/test_task.bat

 echo ^<?php $conn_str = "host=127.0.0.1 dbname=test port=5432 user=%PGUSER% password=%PGPASSWORD%"; ?^> >> "./ext/pgsql/tests/config.inc"
 set PDO_PGSQL_TEST_DSN=pgsql:host=127.0.0.1 port=5432 dbname=test user=%PGUSER% password=%PGPASSWORD%
-set TMP_POSTGRESQL_BIN=%PGBIN%
-"%TMP_POSTGRESQL_BIN%\createdb.exe" test


this incredibly weird command, that doesn't explain what it does at all was replaced by very readable SQL.

ext/sysvshm/tests/bug72858.phpt

Lewiscowles1986 · 2023-10-04T10:23:16Z

Closing as discussed on StackOverflow PHP chat @iluuu1994 doesn't want to share scripts between CI and local-dev

Lewiscowles1986 added 2 commits September 23, 2023 08:32

gitignore: CI files

3b99e74

github-actions bot added Category: Build System Extension: standard Extension: sysvshm labels Sep 23, 2023

Lewiscowles1986 force-pushed the refactor/windows-build branch from 0de4108 to 71edf4d Compare September 23, 2023 08:15

Lewiscowles1986 marked this pull request as ready for review September 23, 2023 08:32

Lewiscowles1986 requested review from iluuu1994, bukka and TimWolla as code owners September 23, 2023 08:32

Lewiscowles1986 mentioned this pull request Sep 23, 2023

single automated Windows build command, ext/skeleton addons #11125

Closed

Lewiscowles1986 added 3 commits September 23, 2023 10:41

refactor: move windows build and test scripts out of GitHub actions s…

8a5b31a

…pecific folder

refactor: pin to CI, instead of GitHub actions

5923973

feat: additional local-build scripts and README

5ae8f18

Lewiscowles1986 force-pushed the refactor/windows-build branch 5 times, most recently from d2a3a25 to 56fc089 Compare September 23, 2023 10:26

TimWolla removed their request for review September 23, 2023 13:01

Lewiscowles1986 force-pushed the refactor/windows-build branch from 56fc089 to e65960e Compare September 24, 2023 00:48

Lewiscowles1986 force-pushed the refactor/windows-build branch 7 times, most recently from 47bba83 to 49132c8 Compare September 24, 2023 09:31

fix: iterate psql setup

4c1fa78

Lewiscowles1986 force-pushed the refactor/windows-build branch from 49132c8 to 4c1fa78 Compare September 24, 2023 09:33

Lewiscowles1986 commented Sep 24, 2023

View reviewed changes

iluuu1994 reviewed Sep 25, 2023

View reviewed changes

Lewiscowles1986 commented Sep 25, 2023

View reviewed changes

Lewiscowles1986 requested review from iluuu1994 and DaveRandom September 27, 2023 13:34

iluuu1994 reviewed Oct 3, 2023

View reviewed changes

ext/sysvshm/tests/bug72858.phpt Outdated Show resolved Hide resolved

bring test_task.bat in-line with master

c36de6e

This comment was marked as resolved.

Sign in to view

revert: GITHUB_ACTIONS -> CI in php test code

73be1b8

github-actions bot removed Extension: standard Extension: sysvshm labels Oct 4, 2023

Lewiscowles1986 closed this Oct 4, 2023


		## build.bat

		The purpose of this script is to compile PHP, after setting up environment. It is intended to be run via command-line `cmd` under windows as an administrator.

refactor: windows build #12281

refactor: windows build #12281

Uh oh!

Conversation

Lewiscowles1986 commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Girgias commented Sep 23, 2023

Uh oh!

Lewiscowles1986 commented Sep 24, 2023

Uh oh!

Lewiscowles1986 commented Sep 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lewiscowles1986 Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lewiscowles1986 Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lewiscowles1986 Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Lewiscowles1986 commented Oct 4, 2023

Uh oh!

Uh oh!

Lewiscowles1986 commented Sep 23, 2023 •

edited

Loading

Lewiscowles1986 commented Sep 24, 2023 •

edited

Loading

Lewiscowles1986 Sep 25, 2023 •

edited

Loading

Lewiscowles1986 Sep 25, 2023 •

edited

Loading

Lewiscowles1986 Sep 25, 2023 •

edited

Loading