search

Level / classic-level

An abstract-level database backed by LevelDB.
MIT License
59 stars 12 forks source link

Swap linux-arm build to use `linux-arm64-lts` #71

Closed cswendrowski closed 1 year ago

cswendrowski commented 1 year ago

It took a lot of time to build and verify, but I was able to confirm this brings down the compiled GLIBC.

Closes #69.

vweevers commented 1 year ago

Do you reckon this can be released as semver-patch? Is there any reason that downgrading glibc could be a breaking change?

cswendrowski commented 1 year ago

Do you reckon this can be released as semver-patch? Is there any reason that downgrading glibc could be a breaking change?

So far as I'm aware from my testing, compiling on a lower GLIBC is only more inclusive of targets, not less. The compilation would have failed if we had too old a GLIBC, so a patch should be fine

vweevers commented 1 year ago

Alright, and we won't need the openssl_fips fix (https://github.com/Level/classic-level/issues/69#issuecomment-1472766966)?

cswendrowski commented 1 year ago

Alright, and we won't need the openssl_fips fix (https://github.com/Level/classic-level/issues/69#issuecomment-1472766966)?

We won't know until we run the Github release action - I needed it to do builds locally, but your Action has previously worked without that update, and my hope is it will do so again. If that fails, we'll need to submit that PR upstream to both leveldb and snappy or otherwise apply those changes when building, which doesn't feel right but I have no idea what else to try

vweevers commented 1 year ago

Added https://github.com/Level/classic-level/commit/89397fd0cedf5637ffac866f7e2c7505ddcdbfde and running it on main now.

vweevers commented 1 year ago

Failed with the openssl_fips error. I reckon it worked previously because the docker images were using an older node version.

vweevers commented 1 year ago

In any case it's unrelated to this PR so I'll go ahead and merge this.

Could you send another PR to fix the *.gyp files? That solution is fine because the *.gyp files are ours. Thank you!

cswendrowski commented 1 year ago

Added 89397fd and running it on main now.

Darn, seems like the same issue: https://github.com/Level/classic-level/actions/runs/4471788122/jobs/7857118969#step:5:48

I have no idea why this would have cropped up since the last build - these are the same dependency versions, and this failed on android-arm, which is unchanged, so I can only assume some environmental difference is now at play.

Some more research reveals it may be the case that Node 14 previously included an openssl_fips, but Node 16 no longer does: https://github.com/nodejs/node-gyp/issues/2750#issuecomment-1286760004

So it seems like it should be possible to add that variable to the node process.config rather than make changes upstream. Will investigate further

vweevers commented 1 year ago

@cswendrowski In case you missed it because we wrote our comments at the same time: https://github.com/Level/classic-level/pull/71#issuecomment-1476771153

cswendrowski commented 1 year ago

In any case it's unrelated to this PR so I'll go ahead and merge this.

Could you send another PR to fix the *.gyp files? That solution is fine because the *.gyp files are ours. Thank you!

Ah neat, I hadn't noticed those weren't in the submodule. PR open! https://github.com/Level/classic-level/pull/72