Skip to content

Use --target instead of building with each node version #80

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

Closed
wants to merge 1 commit into from

Conversation

zbjornson
Copy link
Member

I have not tested this, but this should avoid the issue with Appveyor being slow to update their images and speed up the builds.

I don't know what $NODEJS_VERSIONS looks like; the --target option takes a full vX.Y.Z string, not just X. If you're only using X you could change it to --target=v$ver.0.0.

Copy link
Member

@chearon chearon left a comment

Choose a reason for hiding this comment

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

I know I tried this before and I didn't like the fact that node-gyp --target requires a full version string (edit: ah you said that in the description!). So we have to specify builds as 12.x.x 11.x.x 10.x.x in the environment variables instead of 12 11 10.

It'll be another burden but probably worth it.

Hopefully there weren't any other issues I'm not recalling. After the comment is addressed I'll give it a test run.

.travis.yml Outdated
- >-
source ~/.nvm/nvm.sh
nvm use 10 || { nvm install 10; nvm use 10; }
npm install -g node-gyp
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it was moved from the old node_version.sh file, could you do the nvm commands in ci/linux|osx/preinstall.sh? At that point NVM is already setup.

I think you would just add

nvm install node # latest
nvm use node # latest
npm install -g node-gyp 

Could do Windows node installation in win/preinstall.sh for consistency, or leave it in appveyor.yml since it was already there.

@chearon
Copy link
Member

chearon commented Jun 14, 2019

  1. I was wrong about ci/*/preinstall.sh being ready for NVM, that was true for Linux but not macOS, I've made the fix to .travis.yml for the latter.
  2. I just remembered the reason why I didn't do this. In order to test if the build worked we need node.

node -e "require('./node-canvas')" || {
echo "error loading binary";
exit 1;
}

So we probably do need ci/*/node_version.sh so it can use Install-Product or whatever it does in master, and it can now just do a simple nvm install/nvm use in macOS and Linux. Then I guess we'll have to add something that skips the testing if a node version isn't available 😕 I'm not sure what else we can do unless there's some alternate to NVM for Windows.

@zbjornson
Copy link
Member Author

zbjornson commented Jun 14, 2019

Ahh. What about downloading the portable node executables from https://nodejs.org/dist/vX.Y.Z for testing?

e.g.

https://nodejs.org/dist/v10.16.0/win-x64/node.exe

@chearon
Copy link
Member

chearon commented Jun 14, 2019

That should work yeah, ci/win/node_version.sh can use wget, then probably has to do alias node ='./vX.Y.Z.exe' (hopefully that doesn't have side effects 😬). Note that I changed your example wiki code a bit, the version variable doesn't have the v in it.

I still like using node-gyp's version argument though, we can leave that.

@fpauser
Copy link

fpauser commented Oct 16, 2019

Any news on this?

@zbjornson
Copy link
Member Author

Moot since Automattic/node-canvas#1568

@zbjornson zbjornson closed this May 10, 2020
@zbjornson zbjornson deleted the zb/nodever branch May 10, 2020 00:44
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

Successfully merging this pull request may close these issues.

3 participants