Closed hajimehoshi closed 1 year ago
Shouldn't it be possible to limit the number of env vars before starting go test
? We can get a dump of all env vars and unset the ones we don't need.
It's possible, but 1) it is a little troublesome to do multiple trials with GitHub Actions, and 2) the situation might be changed anytime later so unsetting some environment variables would not be a long-term solution.
Right, but that seems like the only non-hacky solution. My aim with wasmbrowsertest is for the tool to be as dumb as possible without much hacks. It should be possible to add a flag to pass a custom wasm_exec.js, but I am trying to weigh if this problem warrants such a solution.
The environment variable was:
ProgramFiles(x86)=C:\Program Files (x86) CommonProgramFiles(x[8](https://github.com/hajimehoshi/ebiten/actions/runs/3449948187/jobs/5758224308#step:9:9)6)=C:\Program Files (x86)\Common Files SELENIUM_JAR_PATH=C:\selenium\selenium-server.jar NUMBER_OF_PROCESSORS=2 PROCESSOR_LEVEL=6 GOROOT_1_17_X64=C:\hostedtoolcache\windows\go\1.17.13\x64 CONDA=C:\Miniconda CABAL_DIR=C:\cabal GITHUB_WORKSPACE=D:\a\ebiten\ebiten M2=C:\ProgramData\chocolatey\lib\maven\apache-maven-3.8.6\bin JAVA_HOME_11_X64=C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\11.0.17-8\x64 PGROOT=C:\Program Files\PostgreSQL\14 ChromeWebDriver=C:\SeleniumWebDrivers\ChromeDriver USERDOMAIN_ROAMINGPROFILE=fv-az8[9](https://github.com/hajimehoshi/ebiten/actions/runs/3449948187/jobs/5758224308#step:9:10)-916 IEWebDriver=C:\SeleniumWebDrivers\IEDriver POWERSHELL_UPDATECHECK=Off PROGRAMFILES=C:\Program Files GITHUB_PATH=D:\a\_temp\_runner_file_commands\add_path_c90e6dc9-4b6c-4b59-9a35-cd4cadcb59d6 GITHUB_ACTION=__run_5 MSYSTEM=MINGW64 ChocolateyInstall=C:\ProgramData\chocolatey SBT_HOME=C:\Program Files (x86)\sbt\ PATHEXT=.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC JAVA_HOME=C:\hostedtoolcache\windows\Java_Adopt_jdk\11.0.16-[10](https://github.com/hajimehoshi/ebiten/actions/runs/3449948187/jobs/5758224308#step:9:11)1\x64 GITHUB_RUN_NUMBER=3809 RUNNER_NAME=GitHub Actions 2 GRADLE_HOME=C:\ProgramData\chocolatey\lib\gradle\tools\gradle-7.5.1 PGPASSWORD=root OS=Windows_NT HOMEDRIVE=C: PGBIN=C:\Program Files\PostgreSQL\14\bin ANT_HOME=C:\ProgramData\chocolatey\lib\ant\tools\apache-ant-1.10.12 JAVA_HOME_8_X64=C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.345-1\x64 GITHUB_TRIGGERING_ACTOR=hajimehoshi GITHUB_REF_TYPE=branch M2_REPO=C:\ProgramData\m2 USERDOMAIN=fv-az89-916 ANDROID_NDK=C:\Android\android-sdk\ndk\25.1.8937393 *** PIPX_BIN_DIR=C:\Program Files (x86)\pipx_bin USERPROFILE=C:\Users\runneradmin DEPLOYMENT_BASEPATH=C:\actions GITHUB_ACTIONS=true ANDROID_NDK_LATEST_HOME=C:\Android\android-sdk\ndk\25.1.8937393 GITHUB_SHA=6763d34baeda03aa83b28d49485968265c35638a POWERSHELL_DISTRIBUTION_CHANNEL=GitHub-Actions-win22 DOTNET_MULTILEVEL_LOOKUP=0 GITHUB_REF=refs/heads/issue-2412-wasm ALLUSERSPROFILE=C:\ProgramData RUNNER_OS=Windows CommonProgramW6432=C:\Program Files\Common Files GITHUB_REF_PROTECTED=false HOME=/c/Users/runneradmin USERNAME=runneradmin GITHUB_API_URL=https://api.github.com RUNNER_TRACKING_ID=github_79e38b51-4b7f-4aa1-917b-a6b7644929a5 PLINK_PROTOCOL=ssh RUNNER_ARCH=X64 COMSPEC=C:\Windows\system32\cmd.exe RUNNER_TEMP=D:\a\_temp GITHUB_STATE=D:\a\_temp\_runner_file_commands\save_state_c90e6dc9-4b6c-4b59-9a35-cd4cadcb59d6 PGUSER=postgres APPDATA=C:\Users\runneradmin\AppData\Roaming GITHUB_ENV=D:\a\_temp\_runner_file_commands\set_env_c90e6dc9-4b6c-4b59-9a35-cd4cadcb59d6 SYSTEMROOT=C:\Windows GITHUB_EVENT_PATH=D:\a\_temp\_github_workflow\event.json GITHUB_EVENT_NAME=push LOCALAPPDATA=C:\Users\runneradmin\AppData\Local GITHUB_RUN_ID=3449948187 JAVA_HOME_17_X64=C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\17.0.5-8\x64 ANDROID_NDK_HOME=C:\Android\android-sdk\ndk\25.1.8937393 GITHUB_STEP_SUMMARY=D:\a\_temp\_runner_file_commands\step_summary_c90e6dc9-4b6c-4b59-9a35-cd4cadcb59d6 COMPUTERNAME=fv-az89-916 GITHUB_ACTOR=hajimehoshi GITHUB_RUN_ATTEMPT=1 STATS_RDCL=true COBERTURA_HOME=C:\cobertura-2.1.1 ANDROID_HOME=C:\Android\android-sdk GITHUB_GRAPHQL_URL=https://api.github.com/graphql TERM=xterm-256color LOGONSERVER=\\fv-az89-916 npm_config_prefix=C:\npm\prefix PSModulePath=C:\\Modules\azurerm_2.1.0;C:\\Modules\azure_2.1.0;C:\Users\packer\Documents\WindowsPowerShell\Modules;C:\Program Files\WindowsPowerShell\Modules;C:\Windows\system32\WindowsPowerShell\v1.0\Modules;C:\Program Files\Microsoft SQL Server\130\Tools\PowerShell\Modules\ WIX=C:\Program Files (x86)\WiX Toolset v3.[11](https://github.com/hajimehoshi/ebiten/actions/runs/3449948187/jobs/5758224308#step:9:12)\ GITHUB_SERVER_URL=https://github.com PIPX_HOME=C:\Program Files (x86)\pipx TEMP=/tmp DISPLAY=:99.0 SHLVL=1 PROCESSOR_REVISION=5507 GeckoWebDriver=C:\SeleniumWebDrivers\GeckoDriver DriverData=C:\Windows\System32\Drivers\DriverData ANDROID_SDK_ROOT=C:\Android\android-sdk VCPKG_INSTALLATION_ROOT=C:\vcpkg RUNNER_TOOL_CACHE=C:\hostedtoolcache\windows ImageVersion=20221027.1 COMMONPROGRAMFILES=C:\Program Files\Common Files GITHUB_REF_NAME=issue-24[12](https://github.com/hajimehoshi/ebiten/actions/runs/3449948187/jobs/5758224308#step:9:13)-wasm STATS_PFS=true GITHUB_JOB=test EXEPATH=C:\Program Files\Git\bin PROCESSOR_IDENTIFIER=Intel64 Family 6 Model 85 Stepping 7, GenuineIntel AZURE_EXTENSION_DIR=C:\Program Files\Common Files\AzureCliExtensionDirectory PERFLOG_LOCATION_SETTING=RUNNER_PERFLOG GITHUB_REPOSITORY=hajimehoshi/ebiten GCM_INTERACTIVE=Never EdgeWebDriver=C:\SeleniumWebDrivers\EdgeDriver PGDATA=C:\Program Files\PostgreSQL\14\data ANDROID_NDK_ROOT=C:\Android\android-sdk\ndk\25.1.8937393 PHPROOT=c:\tools\php GOROOT_1_18_X64=C:\hostedtoolcache\windows\go\1.18.7\x64 MAVEN_OPTS=-Xms256m GITHUB_RETENTION_DAYS=90 HOMEPATH=\Users\runneradmin RUNNER_WORKSPACE=D:\a\ebiten GITHUB_ACTION_REPOSITORY= TMP=/tmp PATH=/mingw64/bin:/usr/bin:/c/Users/runneradmin/bin:/c/hostedtoolcache/windows/Java_Adopt_jdk/11.0.16-101/x64/bin:/c/Users/runneradmin/go/bin:/c/hostedtoolcache/windows/go/1.19.2/x64/bin:/c/Program Files/MongoDB/Server/5.0/bin:/c/aliyun-cli:/c/vcpkg:/c/Program Files (x86)/NSIS:/c/tools/zstd:/c/Program Files/Mercurial:/c/hostedtoolcache/windows/stack/2.9.1/x64:/c/cabal/bin:/c/ghcup/bin:/c/tools/ghc-9.4.2/bin:/c/Program Files/dotnet:/c/mysql/bin:/c/Program Files/R/R-4.2.1/bin/x64:/c/SeleniumWebDrivers/GeckoDriver:/c/Program Files (x86)/sbt/bin:/c/Program Files (x86)/GitHub CLI:/bin:/c/Program Files (x86)/pipx_bin:/c/npm/prefix:/c/hostedtoolcache/windows/go/1.17.[13](https://github.com/hajimehoshi/ebiten/actions/runs/3449948187/jobs/5758224308#step:9:14)/x64/bin:/c/hostedtoolcache/windows/Python/3.9.13/x64/Scripts:/c/hostedtoolcache/windows/Python/3.9.13/x64:/c/hostedtoolcache/windows/Ruby/3.0.4/x64/bin:/c/tools/kotlinc/bin:/c/hostedtoolcache/windows/Java_Temurin-Hotspot_jdk/8.0.345-1/x64/bin:/c/Program Files/ImageMagick-7.1.0-Q16-HDRI:/c/Program Files (x86)/Microsoft SDKs/Azure/CLI2/wbin:/c/ProgramData/kind:/c/Program Files/Microsoft/jdk-11.0.12.7-hotspot/bin:/c/Windows/system32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0:/c/Windows/System32/OpenSSH:/c/Program Files/dotnet:/c/ProgramData/Chocolatey/bin:/c/Program Files/PowerShell/7:/c/Program Files/Microsoft/Web Platform Installer:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/170/Tools/Binn:/c/Program Files/Microsoft SQL Server/[15](https://github.com/hajimehoshi/ebiten/actions/runs/3449948187/jobs/5758224308#step:9:16)0/Tools/Binn:/c/Program Files/OpenSSL/bin:/c/Strawberry/c/bin:/c/Strawberry/perl/site/bin:/c/Strawberry/perl/bin:/c/ProgramData/chocolatey/lib/pulumi/tools/Pulumi/bin:/c/Program Files/TortoiseSVN/bin:/c/Program Files/CMake/bin:/c/ProgramData/chocolatey/lib/maven/apache-maven-3.8.6/bin:/c/Program Files/Microsoft Service Fabric/bin/Fabric/Fabric.Code:/c/Program Files/Microsoft SDKs/Service Fabric/Tools/ServiceFabricLocalClusterManager:/c/Program Files/nodejs:/cmd:/mingw64/bin:/usr/bin:/c/Program Files/GitHub CLI:/c/tools/php:/c/Program Files (x86)/sbt/bin:/c/SeleniumWebDrivers/ChromeDriver:/c/SeleniumWebDrivers/EdgeDriver:/c/Program Files/Amazon/AWSCLIV2:/c/Program Files/Amazon/SessionManagerPlugin/bin:/c/Program Files/Amazon/AWSSAMCLI/bin:/c/Program Files/Microsoft SQL Server/130/Tools/Binn:/c/Program Files/LLVM/bin:/c/Users/runneradmin/.dotnet/tools:/c/Users/runneradmin/.cargo/bin:/c/Users/runneradmin/AppData/Local/Microsoft/WindowsApps RUNNER_PERFLOG=C:\actions\perflog GITHUB_BASE_REF= ProgramW6432=C:\Program Files GHCUP_INSTALL_BASE_PREFIX=C:\ CI=true ImageOS=win22 RTOOLS40_HOME=C:\rtools40 GITHUB_REPOSITORY_OWNER=hajimehoshi GITHUB_HEAD_REF= GITHUB_ACTION_REF= WINDIR=C:\Windows GOROOT_1_[19](https://github.com/hajimehoshi/ebiten/actions/runs/3449948187/jobs/5758224308#step:9:20)_X64=C:\hostedtoolcache\windows\go\1.19.2\x64 GITHUB_WORKFLOW=Test PROCESSOR_ARCHITECTURE=AMD64 PUBLIC=C:\Users\Public GHCUP_MSYS2=C:\msys64 GITHUB_OUTPUT=D:\a\_temp\_runner_file_commands\set_output_c90e6dc9-4b6c-4b59-9a[35](https://github.com/hajimehoshi/ebiten/actions/runs/3449948187/jobs/5758224308#step:9:36)-cd4cadcb[59](https://github.com/hajimehoshi/ebiten/actions/runs/3449948187/jobs/5758224308#step:9:60)d6 SYSTEMDRIVE=C: ProgramData=C:\ProgramData _=/usr/bin/env
This is about 7639 bytes (including new-lines), and probably this exceeds 8192 bytes by adjusting offsets for each variable.
Note that there is a secret environment variable for passwords and it is hard to estimate actual lengths.
Unsetting PSModulePath
didn't solve the issue. Unsetting PATH
might work but usually I cannot unset this when running wasmbrowsertest.
The constant 8192 for the limitation in wasm_exec.js seems to sync with a constant value in a linker so it would be impossible to change this value.
So, I'd like to modify wasm_exec.js
to ignore the passed environment variables, or modify wasmbrowsertest
not to pass environment variables to go.env
. What do you think?
Sorry, I forgot about this. I'll need to think some more on what the right solution should be. I'll keep you posted.
@hajimehoshi - Apologies for the delay. I think we can add a separate env var WASM_SUPPRESS_ENVVARS
that will prevent passing any env vars to the wasm_exec.js file. Would you be fine with that? I don't want to add any further configuration to be able to pass custom env vars. It's either pass everything or nothing.
Actually, I think here is a better solution that does what we are trying to do: https://stackoverflow.com/questions/58467637/how-to-clear-all-env-variables. If you simply wipe off everything before starting the test, then the tool won't get anything to pass on to wasm_exec.js. You can even enable select env vars after the command.
I'll try to reset all the envs later. Thanks,
https://github.com/hajimehoshi/ebiten/actions/runs/3967893436/jobs/6800384499
Hmm, unset doesn't work well... This seems GitHub Action's unknown behavior to me. I'll take a look further.
PATH
is the biggest environment variable, which takes more than 2000 bytesset
didn't work as expected. Echo-ing to $GITHUB_ENV
didn't work as expectedPATH
correctly is very tricky. If this is empty, even go
doesn't work. I called go run
with env GOOS=js GOARCH=wasm PATH="$(which go)\\.." go test -shuffle=on -v ./...
but failedFixing wasm_exec.js
like this worked:
--- a/usr/local/go/misc/wasm/wasm_exec.js
+++ b/./.github/workflows/wasm_exec.js
@@ -506,6 +506,9 @@
const keys = Object.keys(this.env).sort();
keys.forEach((key) => {
${key}=${this.env[key]}
));
});
argvPtrs.push(0);
So, I still want wasmbrowsertest have a feature to filter out an environment variable. If not, I would modify wasm_exec.js by myself.
Can't you directly pass the full path to the go
binary?
Go itself worked with an empty PATH but the test failed:
fork/exec C:\Users\RUNNER~1\AppData\Local\Temp\go-build3027852083\b[10](https://github.com/hajimehoshi/ebiten/actions/runs/3970323713/jobs/6805908918#step:6:11)1\ebiten.test: %1 is not a valid Win32 application.
So, as I have already said, it is pretty tricky to find a minimal value for PATH.
Ah I see. Indeed.
I think it will be too much customization for wasmbrowsertest to filter out a specific environment variable(s). I was thinking like a complete off or pass everything. Otherwise, modifying wasm_exec.js seems like an easy enough solution without modifying wasmbrowsertest.
I was thinking like a complete off or pass everything
So you decided not to do it, right?
Yes, since it is basically a hammer, and not a clean solution to this exact problem.
It appears GitHub Actions rides really close to the maximum env var size limit for Go Wasm execution. Perhaps we should at least document a work-around to unset more variables during test execution?
I managed to fix this with a small Go script: https://github.com/hack-pad/wasmbrowsertest/pull/1/commits/1325983a3fd5b12c3bef6ba20585118a23dffe69 Cross-OS portability in GitHub Actions files is apparently a big pain. Before I got the above working, I had tried loads of different script variants, but Powershell(?) didn't like them.
Despite this working, a built-in way to filter allowed or disallowed environment variables would be a much better solution. I don't feel like this work-around is good enough to simply be documented. 😕
@agnivade Do you think we should reopen?
It appears my PR has hit the same issue and it's using a bog-standard CI environment.
Yeah, as this is a general issue when using GitHub Actions, having a solution in wasmbrowsertest would be nice.
I like your solution :+1: . It keeps things separate from wasmbrowsertest, and it's an opt-in thing that the users can use. Perhaps we can add another binary under ci/stripenv
and use that in CI? To start off, we can go with a hardcoded set of env vars to filter out. Later, we can take the list as params that are passed during execution.
Do you want to send a separate PR for this?
I could open a PR soon 👍
My concern with my work-around is it's pretty much a band-aid. Since Go Wasm binaries have a very small env var + process arg limit, I'm thinking the unconditional copy of all env variables is problematic.
Of course, because we're inside an exec tool, we don't have access to much — other than environment variables and Go's built-in test flags.
Would it be reasonable to add an option to opt-out on wasmbrowsertest itself?
Some options:
WASM_ENV_PREFIX
which only passes through certain prefixed env var names (and strips the prefix). Like an opt-out, but also opts-in to certain ones at the same time.I was willing to do option 1, but it looks like the $PATH variable was compulsory for tests to run for @hajimehoshi. I am reluctant to do option 2. It's too much customization for a limited scope tool like wasmbrowsertest. wasmbrowsertest should not be responsible to manage what env vars are passed. That should be controlled by the user who is executing the tool.
IMO, this should be fixed during starting the test itself by reducing the list of env vars passed. If that's not an option, then this workaround looks fairly reasonable to me. It is effectively what option 2 would achieve, but outside of wasmbrowsertest, which is my main objective.
I think the reason this is tricky is because Go created the smaller max env var + args limit, effectively punting to the exec tool, which is currently punted to the user. Darn.
IMO, this should be fixed during starting the test itself by reducing the list of env vars passed. If that's not an option, then this workaround looks fairly reasonable to me. It is effectively what option 2 would achieve, but outside of wasmbrowsertest, which is my main objective.
Fair enough 👍 Given there's not a simple cross-platform shell/powershell command to unset multiple variables, I think we may require either A) a Go script of some kind or B) a different script for each OS variant. The single Go script seems to be the simplest approach.
I'll start putting together a PR for this Go script. Do you think it should be:
A separate tool might be great for adopters using CI too. Perhaps something like this?
go install github.com/agnivade/wasmbrowsertest/cmd/cleanenv@latest
cleanenv --ignore 'GITHUB_*' -- \
go test ./...
A separate tool might be great for adopters using CI too. Perhaps something like this?
Yep. So basically solution 1.
Ok great! All set now: https://github.com/agnivade/wasmbrowsertest/pull/43
Thank you! I will take a look as soon as I have some time.
Example: https://github.com/hajimehoshi/ebiten/actions/runs/3447009698/jobs/5752563468
There is a limitation of total environment variable lengths in
wasm_exec.js
(https://github.com/golang/go/blob/fcd14bdcbdfbb5b0c79cfecff95291837836a76d/misc/wasm/wasm_exec.js#L523-L525) and apparently the limitation is exceeded in GitHub Actions.wasmbrowsertest
is not to blame, but would it be possible to add a hack to suppress this error, or add a backdoor to pass an originalwasm_exec.js
?