-
Notifications
You must be signed in to change notification settings - Fork 53
Description
Hi 👋
I can't tell you how nice it is to have this package for instructional material. Concerns about what version of node people have are basically gone now. Thank you!
It looks like appveyor can't install node this way though. Here's the relevant output from the first broken build when I added node as a dep to my testing workshop:
> node@8.9.4 preinstall C:\projects\testing-workshop\node_modules\node
20> node installArchSpecificPackage
21
22npm ERR! code EBADPLATFORM
23npm ERR! notsup Unsupported platform for node-win-x86@8.9.4: wanted {"os":"win32","arch":"x86"} (current: {"os":"win32","arch":"ia32"})
24npm ERR! notsup Valid OS: win32
25npm ERR! notsup Valid Arch: x86
26npm ERR! notsup Actual OS: win32
27npm ERR! notsup Actual Arch: ia32
What's interesting is because the arch is ia32 it should hit this logic:
And the arch variable should be set to x86 (which I believe it is because that's what it tries to install based on the error message). What's surprising to me is that the above line of code seems to indicate that this ia32 should be supported by the x86 package, but it's not.
I'm fairly certain that all that needs to happen is node-win-x86 needs to have its package.json updated to support ia32. It appears that we just need to update this to be an array of the supported archs:
Line 72 in f957051
| cpu: arch |
Which I think could be done by updating this line:
Line 51 in f957051
| const arch = os == 'win' && cpu == 'ia32' ? 'x86' : cpu |
To be this instead:
const arch = os == 'win' && cpu == 'ia32' ? ['x86', 'ia32'] : cpuIf that's right, then I'll go ahead and open up a PR. Though I'm not sure how to backport support to older versions as the version appears to be tied to the version of node 🤔