Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore(core): only install node on Windows if not available #12772

Merged
merged 3 commits into from
Dec 6, 2024
Merged

chore(core): only install node on Windows if not available #12772

merged 3 commits into from
Dec 6, 2024

Conversation

ermshiperete
Copy link
Contributor

@ermshiperete ermshiperete commented Dec 4, 2024 •
edited
Loading

This fixes the build on my Windows machine. Previously nvm install/use for whatever reason caused Windows to no longer be able to find node thus causing the build to fail. Waiting for nvm use to finish solves the build failure. This change when run on Windows additionally first checks the current node version and installs and activates it only if it is not installed.

@keymanapp-test-bot skip

@ermshiperete
chore(core): only install node on Windows if not available
7caf607 
This fixes the build on my Windows machine. Previously `nvm install/use`
for whatever reason caused Windows no longer be able to find `node`
thus causing the build to fail. This change when run on Windows first
checks the current node version and installs it only if it is not
installed.
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Dec 4, 2024 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S16 milestone Dec 4, 2024
@github-actions github-actions bot added common/ common/resources/ Build infrastructure chore core/ Keyman Core labels Dec 4, 2024
@mcdurdin
Copy link
Member

mcdurdin commented Dec 4, 2024

Hmm. Are you using KEYMAN_USE_NVM? I assume so, because otherwise this code never gets run. I'd like to understand why nvm is failing here because this may come back to bite us when we do need to switch node versions.

@ermshiperete
Copy link
Contributor Author

ermshiperete commented Dec 5, 2024 •
edited
Loading

Hmm. Are you using KEYMAN_USE_NVM? I assume so, because otherwise this code never gets run. I'd like to understand why nvm is failing here because this may come back to bite us when we do need to switch node versions.

Yes, I'm using KEYMAN_USE_NVM.

Here's the output of a failing build after running git clean -dxf:

$ ./build.sh build:core
[/d/develop/keyman/keyman] build.sh parameters: <build:core>
[core] child build, parameters: <--builder-child --deps build>
[core] Automatically running following required actions with missing outputs:
[core] * configure:wasm
[core] * configure:x86
[core] * configure:x64
[core] ## configure:wasm starting...
[common/tools/hextobin] dependency build, started by core
[common/tools/hextobin] build.sh parameters: <configure build --deps --builder-dep-parent core>
[common/tools/hextobin] ## configure starting...
Version 20.16.0 is already installed.
Now using node v20.16.0 (64-bit)
/d/develop/keyman/keyman/resources/shellHelperFunctions.sh: line 299: node: command not found

[common/tools/hextobin] Attempted to select node.js version 20.16.0 but found  instead

[common/tools/hextobin] ## configure failed
[core] ## Dependency failed with exit code 1
[core] ## configure:wasm failed
[/d/develop/keyman/keyman] ## build:core failed with exit code 1

@ermshiperete
Copy link
Contributor Author

Adding sleep 1 after the nvm use line makes things work!

@ermshiperete
chore(core): wait for nvm use to finish
fbc3790 
This solves the underlying problem of builds not working on my machine.
Seems the environment isn't fully set after calling `nvm use`, so we have
to either `sleep 1` or wait for the process to end (which this change
does).
@github-actions github-actions bot added core/ Keyman Core and removed core/ Keyman Core labels Dec 5, 2024
mcdurdin
mcdurdin approved these changes Dec 5, 2024
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This is probably related to elevation -- the elevation cycle probably breaks the wait on nvm.exe so it proceeds to the next command in the script. This is an adequate patch, may be worth persevering on the symlink configuration to avoid elevation.

resources/shellHelperFunctions.sh Outdated Show resolved Hide resolved
@mcdurdin mcdurdin mentioned this pull request Dec 5, 2024
Closed
@mcdurdin
Copy link
Member

mcdurdin commented Dec 5, 2024

cross ref on coreybutler/nvm-windows#1183

@github-actions github-actions bot added core/ Keyman Core and removed core/ Keyman Core labels Dec 6, 2024
@ermshiperete ermshiperete merged commit a420e7c into master Dec 6, 2024
3 checks passed
@ermshiperete ermshiperete deleted the chore/core/windows branch December 6, 2024 08:09
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.154-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdurdin mcdurdin mcdurdin approved these changes

@jahorton jahorton Awaiting requested review from jahorton jahorton is a code owner

@srl295 srl295 Awaiting requested review from srl295 srl295 is a code owner

Assignees
No one assigned
Labels
chore common/resources/ Build infrastructure common/ core/ Keyman Core
Projects
Keyman
Archived in project
Milestone
A18S16
Development

Successfully merging this pull request may close these issues.
3 participants
@ermshiperete @mcdurdin @keyman-server