-
Notifications
You must be signed in to change notification settings - Fork 264
Update to Go 1.17.6, Node.js 16.13.2 #5084
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
Conversation
Changed .go-version, then ran `yarn generate`.
matt-boris
left a comment
There was a problem hiding this 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
taskcluster/workers/docker-worker/release.sh
Lines 37 to 54 in cde6847
| # 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`
|
Progress report at the end of my day: go 1.17.6 had significant changes with Node.js 15.0 changed the way unhandled Promise rejects are handled. From the changelog:
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 It appears that |
|
That change to the monitor library sounds like the right choice.
I hear that! |
| @@ -1,3 +1,4 @@ | |||
| //go:build linux || darwin || freebsd | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I'm going to split this up in some new PRs. I think the order is:
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. |
|
The next batch of work has been in other PRs. PR #5088 updated to Go 1.17.6, which also included:
It has been nice to run I'm making progress on #5095, which includes:
I think that's everything from code review on this PR, so I'm going to close this one. |
yarn generatework on my M1 MacBook (arm64)I've built and pushed the new Docker images based on these updates.
Github Bug/Issue: Fixes #5070