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

build: Bump Dockerfile base image to node:20-bookworm #511

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

loewenheim
Copy link
Contributor

@loewenheim loewenheim commented Feb 8, 2024

Another attempt at #472, which was reverted in #475. This time the proximal reason is to get twine 4.0.

I'm assuming that

FROM node:14-buster-slim as builder

at the start of the Dockerfile can stay intact, is that correct?

@loewenheim loewenheim self-assigned this Feb 8, 2024
@@ -15,7 +15,7 @@ RUN \
PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/lib/node_modules/.bin" \
yarn --modules-folder /usr/local/lib/node_modules build

FROM node:14-bullseye
FROM node:20-bookworm
Copy link
Member

Choose a reason for hiding this comment

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

the top of the dockerfile should change too -- so we're building and running on a consistent node version. I'm actually not sure why it was different to begin with though

@@ -11,7 +11,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
node: ['14', '16']
node: ['14', '16', '20']
Copy link
Member

Choose a reason for hiding this comment

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

we can just drop support for node 14 and 16

Copy link
Member

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

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

the tricky part is making sure we don't' regress what was regressed last time -- have you been able to figure out what failed last time this was attempted?

@loewenheim
Copy link
Contributor Author

have you been able to figure out what failed last time this was attempted?

I read through the incident on Jira and the post mortem document, but it's not clear at all why JavaScript publishing failed :/

@asottile-sentry
Copy link
Member

have you been able to figure out what failed last time this was attempted?

I read through the incident on Jira and the post mortem document, but it's not clear at all why JavaScript publishing failed :/

I guess we can try a release after this gets released and see what breaks. and if it does write it down this time 😆

@loewenheim loewenheim enabled auto-merge (squash) February 8, 2024 15:59
@loewenheim loewenheim merged commit 58ef184 into master Feb 8, 2024
8 checks passed
@loewenheim loewenheim deleted the build/node20-bookworm branch February 8, 2024 16:00
@romtsn
Copy link
Member

romtsn commented Feb 9, 2024

I guess we can try a release after this gets released and see what breaks. and if it does write it down this time 😆

I think this broke again 😅 see: https://github.com/getsentry/publish/actions/runs/7841572819/job/21398171351#step:10:130

looks like zlib of node cannot unzip archives

loewenheim added a commit that referenced this pull request Feb 9, 2024
@asottile-sentry asottile-sentry mentioned this pull request Feb 27, 2024
@asottile-sentry
Copy link
Member

here's the actual output so it is not lost with logs going away:

[debug] [[target/maven]] Extracting sentry-android-gradle-plugin-4.3.0.zip:  /tmp/craft-CN8v6B/sentry-android-gradle-plugin-4.3.0.zip
Error:  invalid block type
  at Zlib.zlibOnError [as onerror] (node:zlib:189:17)

in theory that zip is the same one that's here: https://github.com/getsentry/sentry-android-gradle-plugin/releases/tag/4.3.0

@asottile-sentry
Copy link
Member

it's not that zip unfortunately... and the zip that reproduces it is too large to upload here unfortunately :S

@asottile-sentry
Copy link
Member

the zip linked in that github actions run triggers it -- but I'm afraid that will not last forever. going to try and make a reproduction of that but smaller

@asottile-sentry
Copy link
Member

ok here's a small reproduction using the above zip (renamed to inner.zip)

// t.mjs

import unzipper from 'unzipper';

const archive = await unzipper.Open.file(process.argv[2]);
await archive.extract({
  path: process.argv[3],
  concurrency: 2,
});
zip outer.zip inner.zip
node t.mjs outer.zip out-1
node t.mjs out-1/inner.zip out-2

on node 18.12:

$ node t.mjs outer.zip out-1
$ node t.mjs out-1/inner.zip out-2
$

on node 20.11.1

root@4aa4f1b142d7:/y# node t.mjs outer.zip out-20-1
root@4aa4f1b142d7:/y# node t.mjs out-20-1/inner.zip out-20-2
node:internal/process/esm_loader:34
      internalBinding('errors').triggerUncaughtException(
                                ^

Error: invalid block type
    at Zlib.zlibOnError [as onerror] (node:zlib:189:17) {
  errno: -3,
  code: 'Z_DATA_ERROR'
}

Node.js v20.11.1

ended up bisecting this to nodejs/node@654b747

which conveniently just got fixed in nodejs a few days ago in nodejs/node#51993

unzipper seems flawed in general though -- it uses a streaming api for zips which isn't really possible because headers are at the end of the file. I'm going to look into switching unzipping libraries

I tried to make a smaller zip which reproduced the problem, but I think it needs to be a large zip perhaps?

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.

3 participants