Skip to content

Conversation

@jwhitlock
Copy link
Contributor

  • Update Go 1.16.7 to 1.17.6. This makes yarn generate work on my M1 MacBook (arm64)
  • Update Node.js from 14.17.15 to 16.13.2, the LTS version until 2022-10-18.
  • Add notes for how to update Go and Node.js

I've built and pushed the new Docker images based on these updates.

Github Bug/Issue: Fixes #5070

@jwhitlock jwhitlock requested a review from a team as a code owner January 25, 2022 19:43
@jwhitlock jwhitlock requested review from lotas and matt-boris January 25, 2022 19:43
Copy link
Contributor

@matt-boris matt-boris left a comment

Choose a reason for hiding this comment

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

Looking good! Just some small failing tasks. Left a couple small comments.

I believe we can now get rid of the --insecure flag passed to curl command in the docker-worker release script

# TODO: remove --insecure from command below once either the following command
# runs successfully (implies issue fixed upstream in node:14.17.5 docker image):
#
# docker run -ti --rm node:14.17.5 curl -LI https://yarnpkg.com/latest.tar.gz
#
# or once we have upgraded to node 14.18.2 or higher (where this issue is no
# longer present).
#
# The above docker command currently fails with:
# "curl: (60) SSL certificate problem: certificate has expired"
# which is why we added `--insecure` to the command. The issue is with the
# certificate bundle in the node:14.17.5 docker image.
#
# Note, this is also reported in https://github.com/yarnpkg/yarn/issues/8700
# but it is _not_ a problem with yarnpkg/yarn - instead it is a problem with
# node:14.17.5 docker image, so the status of that issue might not reflect
# the status of the required fix.
curl --insecure -L https://yarnpkg.com/latest.tar.gz | tar --transform 's|yarn-[^/]*/|yarn/|' -C $DW_ROOT -zvxf -

From the release notes:

> As of Node.js 15, the default mode for `unhandledRejection` is changed
> to `throw` (from `warn`). In throw mode, if an `unhandledRejection`
> hook is not set, the `unhandledRejection` is raised as an uncaught
> exception. Users that have an `unhandledRejection` hook should see no
> change in behavior, and it’s still possible to switch modes using the
> `--unhandled-rejections=mode` process flag.

The Node.js 14 behaviour is available with `--unhandled-rejections=warn`
@jwhitlock
Copy link
Contributor Author

Progress report at the end of my day:

go 1.17.6 had significant changes with go fmt. yarn lint:go checks the formatting, but fails if there are differences. I tried to extract the formatting commands from the build step.

Node.js 15.0 changed the way unhandled Promise rejects are handled. From the changelog:

Throw On Unhandled Rejections - (issue reference)

As of Node.js 15, the default mode for unhandledRejection is changed to throw (from warn). In throw mode, if an unhandledRejection hook is not set, the unhandledRejection is raised as an uncaught exception. Users that have an unhandledRejection hook should see no change in behavior, and it’s still possible to switch modes using the --unhandled-rejections=mode process flag.

This broke a monitor library test. My reading of the test code is that it is reporting what happens in the default case, so I changed the test to current behavior, rather than try to replicate the old behavior by calling with --unhandled-rejections=warn or adding a unhandledRejection hook in the tested mode (the monitor library has settings to add this hook).

It appears that docker-worker-chunk-5 is failing due to fs-ext not building. This may be because node-gyp finds Python 3.4.3 and needs >=3.6.0. I tried updating workers/docker-worker/Dockerfile, but that didn't seem to help and I'll probably undo that change. There is so much broken in the docker-worker, I find it hard to tell what are "known errors" and what are breaking errors.

@djmitche
Copy link
Collaborator

That change to the monitor library sounds like the right choice.

There is so much broken in the docker-worker, I find it hard to tell what are "known errors" and what are breaking errors.

I hear that!

@lotas lotas mentioned this pull request Jan 26, 2022
1 task
@@ -1,3 +1,4 @@
//go:build linux || darwin || freebsd
Copy link
Member

Choose a reason for hiding this comment

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

Do we need both forms of the build constraint (or can we delete line 2)?

Note, this comment applies to everywhere there is the syntax change in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the new build constraints draft design I think we can get away with just the new format, and delete the old lines. I'm not sure if the draft design was replaced with a final version, but I presume there should be no reason to keep both forms.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, reading the Compatibility section at the bottom of the page, I see it is intended that both forms are preserved, to enable backward compatibility (presumably so people can continue compiling with an older go version, or importing code as a library). Since we require the newer go version, and this isn't library code that others import, I think it should be safe to remove the lines with the older syntax.

@jwhitlock jwhitlock marked this pull request as draft January 26, 2022 15:51
@jwhitlock
Copy link
Contributor Author

I'm going to split this up in some new PRs. I think the order is:

  • Go 1.17.6 update with new developer notes - fixes CI taskgraph failing due to Python 2.7 running Python 3 code #5070 for us M1 / arm64 users
  • Update the CI Docker images - At least worker-ci is on ubuntu:14.04. I think an upgrade to 20.04 is a good idea in general, and will be supported until 2025. It looks like the newer versions of node-gyp require Python 3.6, and this may get us closer.
  • Update to Node.js 16.13.2. This should be smoother once the first two are done.

This may be more of a week-long project than a one-day project, but I think it is possible. Feel free to merge any npm package updates, they will not block the effort until we start on the Node.js update.

@jwhitlock
Copy link
Contributor Author

The next batch of work has been in other PRs. PR #5088 updated to Go 1.17.6, which also included:

  • Adding the developer upgrade notes
  • Removing the older build constraint format.

It has been nice to run yarn generate on my new laptop, and seems to run smoothly in CI.

I'm making progress on #5095, which includes:

  • Updating to Node 16.13.2
  • Removing the temp fix for Node 14.17.5
  • Updating a test for node 1.15.0 behavior
  • Updating worker-ci to Ubuntu 20.04. This image is tagged with the Node version, so it makes sense to me to update it in the same PR. All the docker-worker-chunk-X tests pass, except for one, which reproduces in the https://github.com/taskcluster/docker-exec-websocket-client library.

I think that's everything from code review on this PR, so I'm going to close this one.

@jwhitlock jwhitlock closed this Feb 1, 2022
@jwhitlock jwhitlock deleted the go-17-node-16 branch February 1, 2022 14:24
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.

CI taskgraph failing due to Python 2.7 running Python 3 code

5 participants