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

Prebuild support #19

Merged
merged 4 commits into from
Jul 21, 2018
Merged

Conversation

wingrunr21
Copy link
Contributor

This adds support to prebuild binaries via prebuild. This will prebuild binaries in Darwin, Linux, and Windows (32 and 64 bit) for all supported targets in the node-api project. You can see how the binaries end up being built/uploaded on my test release version v0.12.16.2.

I'm hoping the Travis Linux OS is enough to provide support for Linux electron builds. If additional binaries are required the TravisCI config might have to be modified to build under additional Linux targets.

prebuild-install is used to download the prebuilt binaries via a custom install script.

Only configuration that is needed would be for you to add a Github Personal Access Token with the repo scope as a secure environment variable named PREBUILD_UPLOAD in both TravisCI and AppVeyor. See the prebuild docs as to why.

This resolves #16

@maxbrunsfeld
Copy link
Contributor

Thanks for setting this up! With the current script, is it going to try to upload to GitHub releases on every build? I'd like it to only do that for commits that bump the version number, similar to what prebuild-ci does.

I saw you opened an issue on that repo. Is that the reason that you're not using the module? If so, should we try to replicate a portion of that functionality here in order to avoid redundant uploads?

@wingrunr21
Copy link
Contributor Author

It'll try and upload on every commit to master for that release. You're right, probably should use a build filter to only do that when a new tag is committed.

prebuild-ci has a few issues. You can't force a build without bumping the version (eg if Travis or AppVeyor fail for some reason) and it only uses the running version of Node to build against. I opened the issue as there's no version of node that uses ABI 54 so it's impossible to build against Electron 1.7 via that project. They have this nested callback system for building "oddball" Electron versions. I wanted to get this project building against prebuild before pushing a PR to prebuild-ci for that.

@maxbrunsfeld
Copy link
Contributor

You can't force a build without bumping the version (eg if Travis or AppVeyor fail for some reason) and it only uses the running version of Node to build against.

Yeah, maybe it'd be better to do an upload only if the current commit has a release tag using (git tag -l --points-at HEAD)? That way, if you needed to correct a problem on CI after tagging the version, you could update your tag.

@wingrunr21
Copy link
Contributor Author

So just to give an update: tried to mess with using the Travis/AppVeyor deploy steps to only run prebuilds on a per-tag basis but they are proving to be uncooperative (Travis seems to lose the entire NPM installation and AppVeyor's docs regarding a Script deployer are not accurate).

@wingrunr21
Copy link
Contributor Author

Well, I spoke too soon on Travis. Got it working. Now for AppVeyor

@wingrunr21
Copy link
Contributor Author

wingrunr21 commented Jul 20, 2018

@maxbrunsfeld ok, that latest commit should modify the CI configs to have a deploy step for prebuild that only runs on tags

I had to add a small test command as CI won't run the deploy step when the test command fails. There's also a regex filter on the branches that should match tags. I made it super loose (as it'll technically match a branch like vaslkjfals) on purpose as the only consequence would be a CI run. The prebuild run itself is still gated by a tag.

.travis.yml Outdated
- /^v.*$/

script:
- npm test
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought script: npm test would be the default since we have language: node_js. Does it still work if you remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhh, I think Travis threw a script error when that wasn't there. I don't remember if I had the actual test script in package.json at the same time though. I'll test it later.

@maxbrunsfeld
Copy link
Contributor

This looks great other than the one question.

@wingrunr21
Copy link
Contributor Author

@maxbrunsfeld removed the test script section from the CI config. Seemed to still work

@maxbrunsfeld
Copy link
Contributor

Great! I’ll set up the token on Monday.

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.

Interest in prebuilt binaries?
2 participants