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.

Sign up for GitHub

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

fix: instruct prebuild-install to download napi builds #47

Merged
merged 1 commit into from
Mar 1, 2021
Merged

fix: instruct prebuild-install to download napi builds #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/node-modules/nodeModuleCollector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,16 @@ import (
"go.uber.org/zap"
)

type DependencyBinary struct {
NapiVersions []uint `json:"napi_versions"`
}

type Dependency struct {
Name string `json:"name"`
Version string `json:"version"`
Dependencies map[string]string `json:"dependencies"`
OptionalDependencies map[string]string `json:"optionalDependencies"`
Binary* DependencyBinary `json:"binary`

dir string
isOptional int
Expand Down
37 changes: 26 additions & 11 deletions pkg/node-modules/rebuild.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"os"
"os/exec"
"path/filepath"
"strconv"
"strings"

"github.com/alecthomas/kingpin"
Expand Down Expand Up @@ -39,6 +40,7 @@ type DepInfo struct {
Version string `json:"version"`
Optional bool `json:"optional"`
HasPrebuildInstall bool `json:"hasPrebuildInstall"`
NapiVersions []uint `json:"napiVersions"`

parentDir string
dir string
Expand Down Expand Up @@ -193,7 +195,7 @@ func installUsingPrebuild(dependencies []*DepInfo, configuration *RebuildConfigu
}

return func() error {
logger := log.LOG.With(zap.String("name", dependency.Name), zap.String("version", dependency.Version), zap.String("platform", configuration.Platform), zap.String("arch", configuration.Arch),)
logger := log.LOG.With(zap.String("name", dependency.Name), zap.String("version", dependency.Version), zap.String("platform", configuration.Platform), zap.String("arch", configuration.Arch), zap.Uints("napi", dependency.NapiVersions),)
logger.Info("install prebuilt binary")

parentDir := dependency.parentDir
Expand Down Expand Up @@ -257,14 +259,27 @@ func installUsingPrebuild(dependencies []*DepInfo, configuration *RebuildConfigu
}

func createPrebuildInstallCommand(bin string, extraFlag string, dependency *DepInfo, configuration *RebuildConfiguration) *exec.Cmd {
args := []string{
bin,
"--platform="+configuration.Platform,
"--arch="+configuration.Arch,
"--target="+os.Getenv("npm_config_target"),
"--runtime="+os.Getenv("npm_config_runtime"),
"--verbose",
extraFlag,
var args []string
if len(dependency.NapiVersions) > 0 {
args = []string{
bin,
"--platform=" + configuration.Platform,
"--arch=" + configuration.Arch,
"--target=" + strconv.FormatUint(uint64(dependency.NapiVersions[0]), 10),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really want to just take the first N-API version that it finds here? Isn't that a bit tricky? What if a native module supports multiple versions, like v3 and v4? I'm actually curious how prebuild-install finds which version to download, will check later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right it probably should do something more intelligent, but because of abi stability this will produce something that is guaranteed to work while maybe not optimal, assuming the first is the lowest number.
This is definitely something that can be improved, but wasn't necessary for me to get something that worked (and none of the packages I use target multiple napi versions)

Copy link
Contributor

@dennisameling dennisameling Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Bingo! https://github.com/inspiredware/napi-build-utils/blob/master/index.js#L166-L179

This is used by prebuild-install. They look at process.versions.napi and select the highest available N-API version that's compatible with the currently running NodeJS version. Will do a quick check if we can implement something similar here as well :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good news! It's not necessary to provide a target, because prebuild-install will automatically determine the most appropriate N-API version if the given runtime is N-API 🎉: https://github.com/prebuild/prebuild-install/blob/master/rc.js#L48-L50

After removing the --target, it indeed automatically finds the right N-API version:

• install prebuilt binary  name=sharp version=0.27.1 platform=win32 arch=x64 napi= 
  • execute command  command='C:\Program Files\nodejs\node.exe' 'D:\repos\electron-webpack-quick-start\node_modules\prebuild-install\bin.js' --platform=win32 --arch=x64 --runtime=napi 
--verbose --force
                     workingDirectory=D:\repos\electron-webpack-quick-start\node_modules\sharp
  • command executed  executable=C:\Program Files\nodejs\node.exe
                      errorOut=prebuild-install info begin Prebuild-install version 6.0.1
    prebuild-install info looking for cached prebuild @ C:\Users\Dennis\AppData\Roaming\npm-cache\_prebuilds\a790f7-sharp-v0.27.1-napi-v3-win32-x64.tar.gz
    prebuild-install http request GET https://github.com/lovell/sharp/releases/download/v0.27.1/sharp-v0.27.1-napi-v3-win32-x64.tar.gz
    prebuild-install http 200 https://github.com/lovell/sharp/releases/download/v0.27.1/sharp-v0.27.1-napi-v3-win32-x64.tar.gz
    prebuild-install info downloading to @ C:\Users\Dennis\AppData\Roaming\npm-cache\_prebuilds\a790f7-sharp-v0.27.1-napi-v3-win32-x64.tar.gz.25192-369cb427469f5.tmp
    prebuild-install info renaming to @ C:\Users\Dennis\AppData\Roaming\npm-cache\_prebuilds\a790f7-sharp-v0.27.1-napi-v3-win32-x64.tar.gz
    prebuild-install info unpacking @ C:\Users\Dennis\AppData\Roaming\npm-cache\_prebuilds\a790f7-sharp-v0.27.1-napi-v3-win32-x64.tar.gz
    prebuild-install info unpack resolved to D:\repos\electron-webpack-quick-start\node_modules\sharp\build\Release\sharp.node
    prebuild-install info install Successfully installed prebuilt binary!

  • all native deps were installed using prebuild-install  
  • exited          command=app-builder.exe code=0 pid=29008

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"--target=" + strconv.FormatUint(uint64(dependency.NapiVersions[0]), 10),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What runtime will it be using though?
For example if targeting a version of electron based around node 14, and you are running node 10 will that produce the correct result? I would expect it to be run in your node 10, and so get it wrong

Copy link
Contributor

@dennisameling dennisameling Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I just tested this hypothesis by testing this PR without the target being passed to prebuild-install.

I tested with native binary @dennisameling/keytar-temp@7.4.100-beta which has N-API prebuilds for N-API v3, v6 and v7 on Windows x64.

Test: Electron 11, NodeJS 14.15.5

Electron 11.2.3

process.versions.napi = 6

Node 14.15.5

process.versions.napi = 7

prebuild-install now tries to install a binary for N-API version 11.2.3, which is obviously incorrect as it's the Electron version.

• install prebuilt binary  name=@dennisameling/keytar-temp version=7.4.100-beta platform=win32 arch=x64 napi=  �
  • execute command  command='C:\Program Files\nodejs\node.exe' 'D:\repos\my-electron-app\node_modules\prebuild-install\bin.js' --platform=win32 --arch=x64 --runtime=napi --verbose --force
                     workingDirectory=D:\repos\my-electron-app\node_modules\@dennisameling\keytar-temp
  • build native dependency from sources  name=@dennisameling/keytar-temp
                                          version=7.4.100-beta
                                          platform=win32
                                          arch=x64
                                          napi=  �
                                          reason=prebuild-install failed with error (run with env DEBUG=electron-builder to get more information)
                                          error=prebuild-install info begin Prebuild-install version 6.0.1
    prebuild-install WARN This package does not support N-API version 11.2.3
    prebuild-install WARN install prebuilt binaries enforced with --force!
    prebuild-install WARN install prebuilt binaries may be out of date!
    prebuild-install info looking for cached prebuild @ C:\Users\Dennis\AppData\Roaming\npm-cache\_prebuilds\a79a75-keytar-temp-v7.4.100-beta-napi-v11.2.3-win32-x64.tar.gz
    prebuild-install http request GET https://github.com/dennisameling/node-keytar/releases/download/v7.4.100-beta/keytar-temp-v7.4.100-beta-napi-v11.2.3-win32-x64.tar.gz
    prebuild-install http 404 https://github.com/dennisameling/node-keytar/releases/download/v7.4.100-beta/keytar-temp-v7.4.100-beta-napi-v11.2.3-win32-x64.tar.gz
    prebuild-install WARN install No prebuilt binaries found (target=11.2.3 runtime=napi arch=x64 libc= platform=win32)

  • execute command  command='C:\Program Files\nodejs\node.exe' 'C:\Users\Dennis\AppData\Roaming\nvm\v14.15.5\node_modules\npm\bin\npm-cli.js' rebuild --verbose @dennisameling/keytar-temp@7.4.100-beta
                     workingDirectory=
  ⨯ cannot execute  cause=exit status 1

So you're right, we do need to pass the target N-API version. Just for info, prebuild-install also looks at binary.napi_versions in the package's config, so the approach you took here should be totally valid.

Looking only at the first napi_version in the array

This approach is probably OK for now - in most cases, like you mentioned, it should take the lowest available N-API version from binary.napi_versions which should be fine. Looking forward, I already tried to do some research if we can do something similar to https://github.com/inspiredware/napi-build-utils/blob/master/index.js#L166-L179, but then for Electron.

The problem is this:

  • When running an Electron application, Electron's available N-API version can be retrieved with process.versions.napi, just like in Node itself. It returns the highest supported N-API version; 6 for example.
  • However, prebuild-install leverages Electron's package.json to get the Electron version, and uses that to set its target version. The highest supported N-API version isn't mentioned in the package.json, so AFAIK there's no way to retrieve Electron's N-API version without running Electron itself first. lovell/sharp worked around this by explicitly setting npm_config_target=3 in their package.json. However, this won't help them as soon as they need to support multiple N-API versions.

I probably could create an issue in the electron repo for this - we could ask for a static way to receive the highest supported N-API for the installed Electron version. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah an issue with electron would be good. Doesn't really help us here right now as that will only make it into the next version, but at some point in the future it will. They already have electron --version and electron --abi, so adding an electron --napi would be reasonable.

Until that is a thing, we could spin up electron with a very minimal script to extract the napi version
eg electron napi-version.js
napi-version.js:

console.log(process.versions.napi)
process.exit(0)

This wouldn't be hard to do, but I think it would be more appropriate to do on the js side, and feed it in alongside the platform and architecture.

Copy link
Contributor

Choose a reason for hiding this comment

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

Issue created in electron/electron: electron/electron#27842

"--runtime=napi",
"--verbose",
extraFlag,
}
} else {
args = []string{
bin,
"--platform=" + configuration.Platform,
"--arch=" + configuration.Arch,
"--target=" + os.Getenv("npm_config_target"),
"--runtime=" + os.Getenv("npm_config_runtime"),
"--verbose",
extraFlag,
}
}
command := exec.Command(getNodeExec(configuration), args...)
command.Dir = dependency.dir
Expand Down Expand Up @@ -413,9 +428,9 @@ func readHashBang(path string) (string, error) {
if err != nil {
return "", err
}

defer r.Close()

var header [128]byte
bytesRead, err := io.ReadFull(r, header[:])
if err != nil && !errors.Is(err, io.ErrUnexpectedEOF) {
Expand Down
12 changes: 12 additions & 0 deletions pkg/node-modules/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,18 @@ func writeDependencyList(jsonWriter *jsoniter.Stream, dependencyMap *map[string]
}
}

if info.Binary != nil {
jsonWriter.WriteMore()
jsonWriter.WriteObjectField("napiVersions")
jsonWriter.WriteArrayStart()

for _, v := range info.Binary.NapiVersions {
jsonWriter.WriteUint(v)
}

jsonWriter.WriteArrayEnd()
}

jsonWriter.WriteObjectEnd()
}
jsonWriter.WriteArrayEnd()
Expand Down