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

Skip level on debian 8 #751

Closed
wants to merge 2 commits into from
Closed

Conversation

vweevers
Copy link
Contributor

@vweevers vweevers commented Oct 1, 2019

Because the prebuilt binaries of leveldown are not compatible with Debian 8, surfaced in nodejs/node#29504 (comment).

@Trott /cc @ralphtheninja

Checklist
  • npm test passes
  • tests are included
  • contribution guidelines followed
    here

This is useful to skip e.g. "debian-8" without unintentionally
matching other version numbers like the Node.js version.
The prebuilt binary included in leveldown (a dependency of level)
is not compatible with Debian 8 due to an old glibc version.
@codecov-io
Copy link

codecov-io commented Oct 1, 2019

Codecov Report

Merging #751 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #751   +/-   ##
=======================================
  Coverage   96.22%   96.22%           
=======================================
  Files          27       27           
  Lines         874      874           
=======================================
  Hits          841      841           
  Misses         33       33
Impacted Files Coverage Δ
lib/match-conditions.js 93.61% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27e267f...4871b28. Read the comment docs.

@Trott
Copy link
Member

Trott commented Oct 1, 2019

/ping @nodejs/citgm @nodejs/build

@Trott
Copy link
Member

Trott commented Oct 1, 2019

Any idea why it didn't fail at https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2019/nodes=debian8-64? Unfortunately, I can't find a way to confirm that it tested level in that run, but it really should have, so.... ¯\(ツ)

@vweevers
Copy link
Contributor Author

vweevers commented Oct 1, 2019

🤷‍♂ Either I'm wrong about glibc being the issue, or there's a difference between the rackspace-debian8-x64-1 and -2 machines, or that run indeed didn't test level.

@richardlau
Copy link
Member

Any idea why it didn't fail at https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2019/nodes=debian8-64? Unfortunately, I can't find a way to confirm that it tested level in that run, but it really should have, so.... ¯_(ツ)_/¯

Seems to have passed on that run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2019/nodes=debian8-64/testReport/(root)/citgm/level_v5_0_1/

equally ¯\_(ツ)_/¯

@richardlau
Copy link
Member

🤷‍♂ Either I'm wrong about glibc being the issue, or there's a difference between the rackspace-debian8-x64-1 and -2 machines, or that run indeed didn't test level.

Could very well be a difference in machines if pass/fail is consistent depending on where it was run.

@vweevers
Copy link
Contributor Author

vweevers commented Oct 1, 2019

Seems to have passed on that run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2019/nodes=debian8-64/testReport/(root)/citgm/level_v5_0_1/

That one passed because leveldown got compiled from source, didn't use a prebuild. Uhm.

@richardlau
Copy link
Member

richardlau commented Oct 1, 2019

So of the available history it looks like two builds passed and they both ran on test-rackspace-debian8-x64-2 while the failures are on test-rackspace-debian8-x64-1:
image

Seems to have passed on that run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2019/nodes=debian8-64/testReport/(root)/citgm/level_v5_0_1/

That one passed because leveldown got compiled from source, didn't use a prebuild. Uhm.

So I guess the question is why doesn't the prebuild get used on test-rackspace-debian8-x64-2?

@vweevers
Copy link
Contributor Author

vweevers commented Oct 1, 2019

So I guess the question is why doesn't the prebuild get used on test-rackspace-debian8-x64-2?

On npm install, node-gyp-build checks if the prebuild can be loaded. If not it falls back to compilation. It might be that test-rackspace-debian8-x64-1 passes that loading check for some reason and only hits the problematic code on npm test.

Who has more details on these machines?

Short of that, would it be possible to run on both machines again, just to rule out e.g. race issues?

@rvagg
Copy link
Member

rvagg commented Oct 2, 2019

I've done some updates and minor cleanups on both of those machines. The difference in installed packages is here: https://gist.github.com/rvagg/bd20b289a1f3f9f281ece67a622fc065 - it's a diff of dpkg -l from both machines, the -1 machine is first and -2 is second in this diff.

I've had to remove jessie-backports from the apt sources from both. Some of the differences may be explained from backports that existed at the last time each of the machines were updated. What's most interesting is a number of ubuntu packages listed on the -1. I wonder if at some stage we tried to use some ubuntu sources to get upgrades for something, maybe gcc. In which case, we may have a bit of a mixed up state on that machine that's not truly Debian 8. I have no idea why this might cause a rebuild rather than download of binaries. @vweevers do you know what mechanism it's using to determine system compatibility with binaries? What's it inspecting for on the system to determine libc or whatever else it needs?

I'm going to rebuild this server from the original image, I think I can still do that in Rackspace even though it's an old image, and see how I go.

@rvagg
Copy link
Member

rvagg commented Oct 2, 2019

reprovisioned that machine, don't know if it'll have any impact or not.

@vweevers
Copy link
Contributor Author

vweevers commented Oct 2, 2019

do you know what mechanism it's using to determine system compatibility with binaries? What's it inspecting for on the system to determine libc or whatever else it needs?

@rvagg It matches these system properties against a list of prebuilt binaries (included in the npm package; there's no separate download step):

$ find prebuilds -type f
prebuilds/android-arm/node.napi.armv7.node
prebuilds/android-arm64/node.napi.armv8.node
prebuilds/darwin-x64/node.napi.node
prebuilds/linux-arm/node.napi.armv7.node
prebuilds/linux-arm64/node.napi.armv8.node
prebuilds/linux-x64/node.napi.musl.node
prebuilds/linux-x64/node.napi.node
prebuilds/win32-x64/node.napi.node

If a match is found (should be linux-x64/node.napi.node) it's tested by essentially doing require('path/to/linux-x64/node.napi.node'). If that throws, we fall back to compilation.

@rvagg
Copy link
Member

rvagg commented Oct 2, 2019

well, that list doesn't include anything I could imagine varying between those two machines, it's all pretty basic - node version, glibc or musl, uv version, that's all that really matters here and it's only node version that's going to vary. When citgm is run against Node master it's going to likely get a NODE_MODULE_VERSION that you don't have prebuilds for so maybe the discrepancy described here is as simple as comparing citgm runs on master vs an existing release line branch?

@vweevers
Copy link
Contributor Author

vweevers commented Oct 2, 2019

The NODE_MODULE_VERSION shouldn't matter as leveldown uses N-API.

@rvagg
Copy link
Member

rvagg commented Oct 2, 2019

@vweevers shouldn't matter, but var abi = process.versions.modules is in there, that's NODE_MODULE_VERSION, are you able to bypass it somehow?

@vweevers
Copy link
Contributor Author

vweevers commented Oct 2, 2019

shouldn't matter, but var abi = process.versions.modules is in there, that's NODE_MODULE_VERSION, are you able to bypass it somehow?

Yes. node-gyp-build only matches against ABI if a prebuilt binary has e.g. .abi54 in its filename.

IMO we can let go of the question "why doesn't it use the prebuild". There might be a bug somewhere in the tooling, but it's likely not relevant to node core - so we don't have to discuss that here. I'm fairly certain about Debian 8 not being compatible with leveldown prebuilds, and I'm fine with that.

@rvagg
Copy link
Member

rvagg commented Oct 2, 2019

Dare I suggest we just remove debian8 from citgm entirely? It's EOL in less than 12 months.

@targos
Copy link
Member

targos commented Oct 2, 2019

Dare I suggest we just remove debian8 from citgm entirely? It's EOL in less than 12 months.

SGTM

@richardlau
Copy link
Member

Dare I suggest we just remove debian8 from citgm entirely? It's EOL in less than 12 months.

SGTM

Done! (We do still test on debian 9, https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2026/.)

@vweevers
Copy link
Contributor Author

vweevers commented Oct 3, 2019

This can be closed, then.

@vweevers vweevers closed this Oct 3, 2019
@vweevers vweevers deleted the level-skip-debian-8 branch October 3, 2019 07:03
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.

6 participants