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.

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

Support appveyor #45

Open
kentcdodds opened this issue Mar 6, 2018 · 2 comments
Open

Support appveyor #45

kentcdodds opened this issue Mar 6, 2018 · 2 comments

Comments

@kentcdodds
Copy link

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:

> [email protected] preinstall C:\projects\testing-workshop\node_modules\node
20> node installArchSpecificPackage
21
22npm ERR! code EBADPLATFORM
23npm ERR! notsup Unsupported platform for [email protected]: 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:

cpu: arch

Which I think could be done by updating this line:

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

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
Copy link
Owner

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
Copy link
Author

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

kentcdodds pushed a commit to kentcdodds/testing-workshop that referenced this issue Mar 21, 2018
kentcdodds pushed a commit to kentcdodds/testing-workshop that referenced this issue Mar 21, 2018
kentcdodds pushed a commit to kentcdodds/testing-workshop that referenced this issue Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants