fix: Action did not work on Windows runners. #257
Conversation
|
|
sokuhatiku
changed the title
There was a problem hiding this comment.
I added some small comments while reviewing. Overall, this looks good to me. I think it's worth having an additionnal review on this before merging and creating a release.
Thanks a lot for your contributions sokuhatiku! :D
| import { run as main } from './main'; | ||
| import path from 'path'; | ||
| import { run as post } from './post'; | ||
|
|
||
| /* | ||
| * GitHub Action can provide multiple executable entrypoints (pre, main, post), | ||
| * but it is complicated process to generate multiple `.js` files with `ncc`. | ||
| * So we rather generate just one entrypoint, that is symlinked to multiple locations (main.js and post.js). | ||
| * Then when GitHub Action Runner executes it as `node path/to/main.js` and `node path/to/post.js`, | ||
| * it can read arguments it was executed with and decide which file to execute. | ||
| * The argv[0] is going to be a full path to `node` executable and | ||
| * the argv[1] is going to be the full path to the script. | ||
| * In case index.js would be marked executable and executed directly without the argv[1] it defaults to "main.js". | ||
| */ | ||
| async function run([, name = 'main.js']: string[]) { | ||
| const script = path.basename(name); | ||
|
|
||
| switch (script) { | ||
| case 'main.js': | ||
| await main(); | ||
| break; | ||
| case 'post.js': | ||
| await post(); | ||
| break; | ||
| default: | ||
| throw new Error(`Unknown script argument: '${script}'`); | ||
| } | ||
| } | ||
|
|
||
| run(process.argv); | ||
| export { run as main } from './main'; | ||
| export { run as post } from './post'; |
There was a problem hiding this comment.
I'm not too familiar with that specific section. I definitely prefer the one with less lines. This was done in the following commit: dea8579
I assume it is fine, but I'd like @webbertakken's input on this part just in case.
dist/main.js
Outdated
There was a problem hiding this comment.
edit: nevermind, I misread my own change. I tried to build this myself to see if there was a difference with the build and I had a different result than what I see here. See 32df7d8.
I wonder if the Node version used to build has an impact here. branch is fix/windows-runner-dist-644. Don't mind the 644 in there, that's a small misunderstanding on my side which I figured out after writing here.
I personally built on Node v18.19.0.
|
@sokuhatiku Would you mind rebasing and regenerating the dist files? Should be straight forward. I think it would be great to merge this one. |
sokuhatiku
force-pushed
the
fix/windows-runner
branch
from
52b336a
to
d9c2f50
Compare
|
@webbertakken I've rebased and regenerated the dist files as requested. |
Changes
Fix action to work on
windows-latestnographicsoption to Editor command line variable.Added tests to verify that action works on
windows-latestRelated Issues
Successful Workflow Run Link
Notes
-nographicsoption to the Unity command line arguments on windows runner. This change affects tests that use graphics features such as RenderTexture for example.Checklist
in the documentation repo)