search

aredridel / node-bin-gen

157 stars 51 forks source link

Support appveyor #45

Open kentcdodds opened 6 years ago

kentcdodds commented 6 years ago

Hi :wave:

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:

https://github.com/aredridel/node-bin-setup/blob/ad56be2b72108028fc058c07665035e34f25c0e9/index.js#L10

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:

https://github.com/aredridel/node-bin-gen/blob/f957051398f288a7288e3be9f3dc18b463f94704/index.js#L72

Which I think could be done by updating this line:

https://github.com/aredridel/node-bin-gen/blob/f957051398f288a7288e3be9f3dc18b463f94704/index.js#L51

To be this instead:

const arch = os == 'win' && cpu == 'ia32' ? ['x86', 'ia32'] : cpu

If 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 🤔

aredridel commented 6 years ago

Yeah, that's right.

And yeah, that's gonna suck for old versions :( Stupid ambiguity using different ids for that thing in different places.

If one can forcefully install the script could be modified to do that. It bears some investigation. Let's keep brainstorming!

kentcdodds commented 6 years ago

I'm not sure I follow, but I'm happy to open a PR to do whatever you'd prefer :)