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

Update prebuild naming to scheme #25

Closed
mafintosh opened this issue Jan 24, 2019 · 26 comments · Fixed by #27
Closed

Update prebuild naming to scheme #25

mafintosh opened this issue Jan 24, 2019 · 26 comments · Fixed by #27
Labels
enhancement New feature or request

Comments

@mafintosh
Copy link
Collaborator

mafintosh commented Jan 24, 2019

With the added metadata from various PRs plus my personal intent to add stuff like the libuv major version to the name for prebuilds that use that (to ensure abi compatibility) I've been thinking we should make our naming a bit more generic.

Here are my thoughts.

Currently our prebuild names look like this: {runtime}-{napi|abi}.node

I suggest we move to something a bit more explicit like this: {runtime}-{name(.value)?}-{name(.value)?}....node

For example for a napi build for node that tags the uv major version 1: node-napi-uv.1.node
Or one that tags a special libc for abi 40: node-abi.40-libc.musl.node or to tag alpine as the os, node-abi.40-os.alpine.node

The prebuild picker, node-gyp-build would then pick the one with the most tags that match (obvs abi or napi must match first etc).

WDYT? The use of . and - as separators is somewhat arbitrary and I'm open for suggestions

@mafintosh
Copy link
Collaborator Author

/cc @ralphtheninja @vweevers

@ralphtheninja
Copy link
Member

I like the idea a lot because it solves the current use cases and gives more room for future requirements.

I'm more curious how it would work api wise, like how prebuildify should be invoked to get the desired file name and how node-gyp-build should be used to pick/match correctly.

@ralphtheninja ralphtheninja added the enhancement New feature or request label Jan 24, 2019
@mafintosh
Copy link
Collaborator Author

@ralphtheninja We'd add some built in flags like --tag-libuv, that auto sets the version and the user configurable ones with --tag=libc.musl. node-gyp-build would use a set of predefined tags to match internally.

@ralphtheninja
Copy link
Member

node-gyp-build would use a set of predefined tags to match internally.

Aye, I figured we need something to make the interpretation. I'd like to keep the format consistent though, so everything before the ending .node (e.g. var tags = str.split('.node')[0].split('.')) are tags separated with whatever separator we pick (lets go with . for now), including runtime. I also don't think the order should be important (we will generate in a certain order, but technically it shouldn't matter).

So lets drop some tags we want:

  • runtime (string, values: node or electron)
  • abi (string, values: 47, 48 etc)
  • napi (boolean)
  • libc (string, values: glibc or musl)
  • os (string, values: linux, darwin, win32, android etc)
  • uv (string)

Side note: Does os.platform() yield alpine or linux?

@vweevers
Copy link
Member

vweevers commented Jan 25, 2019

everything before the ending .node are tags

You mean e.g. runtime-node.napi.libc-musl.os-linux.uv-1.node (order aside)?

How about defining shortcuts for some of the tags, e.g. node means { runtime: node }, musl means { libc: musl }, linux means { os: linux }. Then we get shorter (and IMO more readable) names (node.napi.musl.linux.uv-1.node) while still having a consistent logic internally.

So lets drop some tags we want:

+ armv
+ arch

Side note: Does os.platform() yield alpine or linux?

linux.

@ralphtheninja
Copy link
Member

Then we get shorter (and IMO more readable) names (node.napi.musl.linux.uv-1.node) while still having a consistent logic internally.

👍

@mafintosh
Copy link
Collaborator Author

@vweevers ya sgtm

@vweevers
Copy link
Member

Re: alpine: older alpine uses glibc, so to keep open the option of supporting older alpine (if and when someones asks for it), I don't think we should use alpine as a tag (value).

@vweevers
Copy link
Member

The prebuild picker, node-gyp-build would then pick the one with the most tags that match (obvs abi or napi must match first etc).

Would it be feasible/helpful to publish some sort of manifest alongside the binaries? Then node-gyp-build would only have to read that file, rather than scanning directories and parsing tags in filenames.

@ralphtheninja
Copy link
Member

Would it be feasible/helpful to publish some sort of manifest alongside the binaries? Then node-gyp-build would only have to read that file, rather than scanning directories and parsing tags in filenames.

What would this manifest look like and how do you see it working?

@vweevers
Copy link
Member

{
  "prebuilds": [{
    "path": "does-the-name-matter-anymore.node",
    "tags": {
      "platform": "linux",
      "arch": "x64",
      "runtime": "node",
      "napi": true
    }
  }]
}
// node-gyp-build pseudo-code
// Ignore paths being relative for a moment
var manifest = JSON.parse(fs.readFileSync('manifest.json', 'utf8'))

// Assume that prebuilds are ordered most-specific first
for (var prebuild of manifest.prebuilds) {
  if (match(prebuild.tags)) {
    return require(prebuild.path)
  }
}

@mafintosh
Copy link
Collaborator Author

Manifest sounds good. We should still make the filenames deterministic though based on the input, so they are auto overwritten if you prebuild twice

@ralphtheninja
Copy link
Member

I like it.

When is this manifest written? If it's supposed to be one manifest, e.g. prebuilds/manifest.json then maybe prebuildify-ci download should assemble it when gathering all prebuilds before npm publish?.

@mafintosh
Copy link
Collaborator Author

I'd put a manifest inside the ./prebuilds/{os} folder so it can all be tarred up and put on s3/github releases and all a download script needs to do is fetch it

@ralphtheninja
Copy link
Member

i'm wondering if we even need the prebuilds/${targetPlatform}-${targetArch} folder structure anymore. Why not just put everything flattened inside prebuilds/? Maybe I'm going too much into details :)

@mafintosh
Copy link
Collaborator Author

I'd keep it seperate as you are building on each of those platforms individually using CI, so if you get rid of the folder you'd have to add a bunch of merging logic to download prebuilds instead of a simple curl + tar

@ralphtheninja
Copy link
Member

I'd keep it seperate as you are building on each of those platforms individually using CI, so if you get rid of the folder you'd have to add a bunch of merging logic to download prebuilds instead of a simple curl + tar

Got it. So then we should keep the same folder structure as we have now.

@vweevers
Copy link
Member

@mafintosh What does the l mean in linux-armv7l.tar of sodium-native?

@mafintosh
Copy link
Collaborator Author

@vweevers it's uname -i from a raspberrypi :D

@mafintosh
Copy link
Collaborator Author

Eh, uname -a. From my pi:

Linux hyperpi 4.14.79-v7+ #1159 SMP Sun Nov 4 17:50:20 GMT 2018 armv7l GNU/Linux

@vweevers
Copy link
Member

Here's the (rough!) plan for leveldown. We're likely gonna tackle Alpine prebuilds at a later time, but it'd be good to take into account now. Manifests might make things harder (see the red note).

leveldown prebuilds 4

@vweevers
Copy link
Member

vweevers commented Jan 26, 2019

Updated to: remove manifests, add --tag arguments (syntax TBD, we can hash that out in a PR), fix arm vs arm64 (per discussion in Level/leveldown#572), add TARGET_PLATFORM (#22) and ARCH variables, add android-arm64 target for illustrative purposes.

leveldown prebuilds 9

@vweevers
Copy link
Member

vweevers commented Mar 9, 2019

As for separators: some options, using a runtime that has dashes in its name (ignore the nonsensical combination of tags):

# Format Example Notes
1 {k(.v)?}- node-webkit-abi.40-armv.8 ❌ Can't easily parse
2 {k(-v)?}. node-webkit.abi-40.armv-8 ❌ Somewhat ambiguous
3 {k(.v)?}_ node-webkit_abi.40_armv.8 ❌ Hard to read or is it me?
4 {k(_v)?}. node-webkit.abi_40.armv_8
5 {k(v)?}. node-webkit.abi40.armv8 Custom tags are not possible.
Do we need them?

@mafintosh
Copy link
Collaborator Author

I don’t personally think we need custom tags in practice. I like 5.

@vweevers
Copy link
Member

Started working on this.

I'm wondering, for tags that we can't always detect the value of (like libc and armv), whether the value should be specified via:

  1. The tag, e.g. prebuildify --tag-armv 7
  2. Options or env, without needing --tag-armv, e.g. --armv 7 or PREBUILD_ARMV=7. I don't like this because you might want to build a specific version, but without tagging that build, assuming that the build is forwards-compatible (e.g. armv8 machines using an armv7 build).
  3. Options or env, e.g. --tag-armv --armv 7 or PREBUILD_ARMV=7 prebuildify --tag-armv
  4. Combination of the above, e.g. !isBoolean(opts.tagArmv) ? opts.tagArmv : opts.armv

I prefer 3 or 4, because like we did for the other options (#26), it'd be nice to also make PREBUILD_ARMV available to subprocesses. And I'm leaning towards 3, because when you're not cross-compiling (running prebuildify on an actual ARM machine) we áre able to detect the armv automatically so you'd only need --tag-armv.

@vweevers
Copy link
Member

Nevermind, 4 is easy to achieve, I'll go with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants