-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add support for distributing prebuilds as platform-specific packages #63
base: master
Are you sure you want to change the base?
Conversation
Awesome, some CI failures above if you can take a look. So with the optional deps, all builds are installed but then discarded at install time if they are irrelevant? |
I'm a little stumped on this. The CI failures seem like they are due to Visual Studio being missing or out-of-date on the CI target machines, and unrelated to any changes I made. I verified that the
Basically yes, although there is actually some slight variation in how this is handled. In NPM v6 and Yarn, all the platform packages are download (into the cache), and the platform compatibility is checked and only the matching/compatible package gets installed (others are discarded). NPM v7+ improves on this, and only downloads the package metadata first, then checks the platform compatibility and then only downloads the matching/compatible platform package. But in all cases, only the compatible/matching platform package gets installed. If you want to see this all in action, I've built and published a beta version of my msgpackr-extract package with this updated system, and you can try it out (with full details so you can see the requests that npm makes):
|
The Windows failure is a mismatch between github runners and node-gyp version, indeed unrelated to this PR. I've seen it before, will have a look. Either need to downgrade github runner or bump node-gyp. |
@kriszyp I fixed windows CI on the master branch; rebasing should do it |
…s individual platform-specific packages to be accessed as optionalDependencies
5be43f8
to
dde2c9f
Compare
dde2c9f
to
4269a27
Compare
33f720c
to
f9fa455
Compare
This pr looks promising. Should there be some tooling to help generate the package.json optionalDependencies block? I can very easily see myself forgetting to update it when publishing new versions, resulting in a broken publish of the main package. I would suggest it best to require the platform packages to be scoped packages, to prevent potential issues with package name squatting. |
That would be nice, but I don't think it belongs here. At least with how I have used prebuildify, it is run in a separate container/server for each platform, so it is more focused on the builds for that platform, without knowledge of other platforms that will be built. I use prebuildify-ci to download all the builds (from github), and I think it would be reasonable to set the optionalDependencies there. However, I am not sure if everyone is using that (or prebuildify-cross?), and it seemed like the easiest part of this to do separately from the prebuildify tools (here is the script I use). But I'd be happy to put together a PR for prebuildify-ci if that helps. The one other thing that requires a little bit of manual effort is setting up the mass publishing, but again, it is not too hard to set up.
Oh yeah, scoped packages are good idea for this. Now I feel bad I haven't been using that. So I think this raises a few questions about configuring and requirements. First, do we include a setting to indicating the scope, or infer it? For unscoped packages, do we assume the platform builds use a scope with the same name as the package, like Or are the some cases where might be preferable to publish under a scope that isn't used by the package? And if so would it be better to have a separate argument for this, or have a single |
Yeah that makes sense. I have not actually usedprebuildify for anything myself yet, due to lack of cmake-js support so I dont have any real knowledge on workflows. I would really like to start using prebuildify or something similar though (and have been for over a year now)..
I think the |
Perhaps something like this?
|
If possible, I'd like to avoid configurable prefixes, as it makes discovery (by |
In The problem I can see with forced prefixes, is that if you publish Or the more concerning thought, of that if you make the scanning for this libs be enabled by default, then that feels like a dll injection attack waiting to happen. I suppose this isnt very likely because you would need to still have the rogue module explicitly installed onto the machine, but it could happen |
I was thinking about that as well. However, I was wondering if it would be better if node-gyp-build actually scanned the list optionalDependencies from the package.json, and then matched on the platform names there. This would also provide a way of dealing with the combined architecture builds that macos supports (and once a package from optionalDependencies was matched that clearly indicates the package to load). But that being said, I am not opposed to simple and predictable :). |
I would rather it didnt do that.. dynamic import of files is the exact way to make libraries not work with webpack or other bundlers.. I have a project where I am playing with bundling in webpack, and all the native modules have had to be kept outside of that bundle (which then includes all their dependencies), often because the clever searching for Making it an explicit call when invoking |
I think supporting only this will work in 99% of cases:
The |
Sounds good to me. So to be clear, no extra arguments, the only change is making unscoped packages map from I guess one other thing we might consider is if we want a little more descriptive package names for the scoped platform names than just "linux-x64", like maybe mapping to I think the only collision hazard would be if someone has a package with the same name as an existing org (or username) (I think they are packages and orgs are different namespaces), but seems reasonable to cross that bridge if it ever actually happens. |
Or even |
Co-authored-by: Vincent Weevers <mail@vincentweevers.nl>
This means the parent package's code is still installed right? Which would mean that the platform-specific dependency should only contain the native-addon binary. Or would it be better to have the js-bindings and native-addon binary in the platform-specific package? |
Yes
Basically yes, although it also needs (and auto-creates) a simple package.json, which is necessary for publishing, and an empty index.js, which is necessary to get node to resolve the package path).
You could, but then the js bindings get duplicated to each platform package, and since some package managers download all the platform packages, would be a little more wasted bytes, less efficient. Also, prebuild's node-gyp-build is set up to just dynamically load the binary, and this PR was intended to have minimal changes to prebuild's system. |
I see so you have 2 options:
|
If it helps at all, you can see this PR (and the node-gyp-build PR) in use with my https://www.npmjs.com/package/msgpackr-extract package, and platform packages at https://www.npmjs.com/org/msgpackr-extract . |
Question, how does Does that mean there are meant to be some sort of constraint either via an I just saw the But what about for things like |
@CMCDragonkai The parent project includes optional dependencies for all the platforms, and each platform package specifies an os and cpu so that npm rejects all but the matching platform dependency package. If there is no matching platform, the parent project has |
A promising technique for distributing prebuilt binaries is to use a set of platform-specific packages for each platform's prebuilt binaries. Then each platform-specific package specifies the required platform and architecture, and the parent package specifies all the platform packages as
optionalDependencies
. When installed, NPM will then only install the optional dependency with the platform/architecture matching the current machine.This provides both the efficiency of only needing to download the binary (or binaries) needed for the current platform, but also the key benefit of the prebuildify system in that binaries are downloaded as part of the normal NPM install process (no install scripts for extra downloads are necessary).
The new parcel css bundler demonstrates this technique:
https://cdn.jsdelivr.net/npm/@parcel/css@1.7.3/package.json
And this has also been suggested by isaacs himself for platform specific distribution:
npm/rfcs#120
As it turns out, this technique actually works really nicely, and with relatively little effort/changes, to the prebuildify system. I attempted to add some minimal modifications to add a flag to enable creating these optional platform-specific packages in this PR.
A few other things would need to be done to finish this: