-
Notifications
You must be signed in to change notification settings - Fork 53
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
Move bin
path, in Windows, from D: to C:
#251
Conversation
@@ -135,7 +135,6 @@ async function maybeInstallGleam(gleamSpec) { | |||
core.startGroup(`Installing Gleam ${gleamVersion}`) | |||
await install('gleam', { toolVersion: gleamVersion }) | |||
core.setOutput('gleam-version', gleamVersion) | |||
core.addPath(`${process.env.RUNNER_TEMP}/.setup-beam/gleam/bin`) |
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.
Is not used: garbage-remainder of a prior change.
@@ -156,7 +155,6 @@ async function maybeInstallRebar3(rebar3Spec) { | |||
core.startGroup(`Installing rebar3 ${rebar3Version}`) | |||
await install('rebar3', { toolVersion: rebar3Version }) | |||
core.setOutput('rebar3-version', rebar3Version) | |||
core.addPath(`${process.env.RUNNER_TEMP}/.setup-beam/rebar3/bin`) |
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.
Is not used: garbage-remainder of a prior change.
'.setup-beam', | ||
toolName, | ||
) | ||
fs.cpSync(cachePath, runnerToolPath, { recursive: true }) |
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 the change that makes for different Windows C: vs. D: behaviour. All other changes are mostly variable renaming for explicitness.
1d02aaa
to
5751e76
Compare
.github/workflows/ubuntu.yml
Outdated
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.
bin
is supposed to not be part of the INSTALL_DIR_...
as per previous expectations.
The move from -v
to --version
serves the purpose of approaching this test to the implemented code.
.github/workflows/windows.yml
Outdated
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.
We now run in PowerShell and use consumer expectations to validate our results: bin
is a prior expectation, changes to arguments approach the test to the code, as for the file above.
dist/index.js
Outdated
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.
Auto-generated, no worries here 🤔 🤣
package-lock.json
Outdated
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.
Auto-generated as per:
rm -f package-lock.json
npm run build-install
👍
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 understand if the changes in version were done upstream (strange, since we move no version in our scope) or via some auto-npm audit
thing. In any case it's strange, but it solves the local npm audit
-based issues, too.
package.json
Outdated
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.
Just minor organisation changes...
This might prevent a class of errors in Windows On the other hand this restore the paths to what we were using prior to introducing the caching behaviour mechanism The important change in the commit is the use of fs.cpSync
We were facing npm audit issues that'd been fixed upstream already, but since the files weren't recreated we'd not notice this
We do this to take advantage of the most recent changes to package.json
Not a good idea do remove it during build-dist...
21b8660
to
4dfb38b
Compare
Released in |
Description
This addresses a Windows-specific issue related to a recent change (the target folder which we add to the path after using
tool-cache
) where the tool runs after downloading from cache. Hopefully moving stuff from C: to D: (where the runner keeps e.g. its temp), in Windows, will solve that class of issues. Check the linked bug report for more discussion elements.I also take this opportunity to do minor changes to the project maintenance files that I'll comment on.
Closes #245.