Improve Windows version detection and HOME variable export#883
Improve Windows version detection and HOME variable export#883eregon merged 3 commits intoruby:masterfrom
Conversation
blacksmith.sh runners use "Windows Server 2025 Standard Evaluation".
index.js
Outdated
| core.exportVariable('TMPDIR', ENV['RUNNER_TEMP']) | ||
| // bash - sets home to match native windows, normally C:\Users\<user name> | ||
| core.exportVariable('HOME', ENV['HOMEDRIVE'] + ENV['HOMEPATH']) | ||
| // Use os.homedir() which calls the Windows API (SHGetFolderPath) directly. |
There was a problem hiding this comment.
This seems to not quite match what the docs say:
On Windows, it uses the USERPROFILE environment variable if defined. Otherwise it uses the path to the profile directory of the current user.
There was a problem hiding this comment.
So are the docs wrong or is this comment inaccurate?
Probably the docs are right and we we just want to use a reliable approach vs depending on env vars directly.
There was a problem hiding this comment.
From what I read online, USERPROFILE is the same as HOMEDRIVE/HOMEPATH for majority of the cases, but it’s not guaranteed. If we want to be safe and not potentially breaking anyone, I think we should do a fallback logic, that first try HOMEDRIVE/HOMEPATH and then fallback to USERPFOFILE.
There was a problem hiding this comment.
We already use os.homedir() for createGemRC() and it seems to work fine so I think using os.homedir() is good enough and correct, just the comment needs to not be contradicting the docs 😅
There was a problem hiding this comment.
The docs are right, and my library research led me in the wrong direction.
The full comment would be:
// Env-based approaches (USERPROFILE, HOMEDRIVE+HOMEPATH) are unreliable on some
// runners (e.g. Blacksmith) where these vars are unset, causing NaN, which
// JSON.stringify serializes to "null".
// os.homedir() checks USERPROFILE first, then falls back to the
// GetUserProfileDirectoryW Windows API when USERPROFILE is unset.
However, the question is whether so long is needed here?
There was a problem hiding this comment.
Cool, in that case it’s actually better to have a consistent usage across different places.
There was a problem hiding this comment.
Yeah I'm also fine to just remove the comment entirely because we then just use the method that node provides. Could you remove it then?
There was a problem hiding this comment.
Changed the comment to the previous one without the path details.
eregon
left a comment
There was a problem hiding this comment.
The changes look good.
Of course since this cannot be tested in this action CI there might be other issues and it's not really an officially supported platform from the POV of setup-ruby maintainers.
But, it should still work because on Windows we use RubyInstaller2 builds which should work on most Windows, and there are hopefully not too many relevant differences between blacksmith.sh runners and GH-hosted Windows runners.
a1da11a to
97d485f
Compare
97d485f to
23fc5fa
Compare
This PR fixes the issues mentioned in #882 with installing Ruby on blacksmith.sh runners.