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

Local prebuilds #137

Merged
merged 7 commits into from
Apr 3, 2021
Merged

Local prebuilds #137

merged 7 commits into from
Apr 3, 2021

Conversation

jchook
Copy link
Contributor

@jchook jchook commented Dec 23, 2020

See #136 for discussion

Rationale

When an arch/platform/version does not have a published prebuild, we currently have two options:

  1. Host an HTTP server with the proper directory structure to serve the missing prebuilds, and set node_config_${package}_binary_host in the build environment
  2. Wait for CI flow to build the native modules

This PR enables users to provide prebuilds via the host's filesystem.

Changes

  • Reinstates code removed by a069253
  • Adds --local-prebuilds flag to specify prebuild dir
  • Adds npm_config_${pkg.name}_local_prebuilds env var to specify prebuild dir
  • Adds tests for local prebuilds

Notes

I mostly just re-added previously existing code. I also added:

  • var + env config for the otherwise hard-coded prebuilds dir
  • tests

@jchook
Copy link
Contributor Author

jchook commented Dec 24, 2020

Please give any feedback, esp suggestions on how to fix the Windows + Node 10 error on AppVeyor

Copy link
Member

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, the CI failure appears to have been temporary as it worked when re-run. I've left one comment inline but otherwise this is looks good to me.

util.js Outdated Show resolved Hide resolved
@jchook
Copy link
Contributor Author

jchook commented Jan 29, 2021

Thanks for the review 👍

@gochax
Copy link

gochax commented Mar 1, 2021

Hi, when can this PR be merged? I would need its functionality quite urgently.

@MikeBauerCA
Copy link

This functionality would benefit our team as well! @lovell is there an intent to merge this PR in the near future? Thanks @jchook for opening 👍

@lovell lovell requested a review from vweevers March 15, 2021 18:19
@vweevers
Copy link
Member

vweevers commented Apr 3, 2021

I don't have time to review and I'm on the fence about whether this feature is in scope, but this is a quality PR (has tests, good description, etc), I trust @lovell, and the upvotes indicate that multiple folks need this feature, so I'm merging and releasing.

@vweevers vweevers merged commit dc4e5ea into prebuild:master Apr 3, 2021
@vweevers
Copy link
Member

vweevers commented Apr 3, 2021

6.1.0

t1m0thyj added a commit to zowe/zowe-cli-scs-plugin that referenced this pull request Apr 5, 2021
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.

5 participants