Upgrade to Node.js v16

[christophebedard]

Closes #519

Signed-off-by: Christophe Bedard [email protected]

[codecov[bot]]

Codecov Report

Base: 93.10% // Head: 93.14% // Increases project coverage by +0.03% :tada:

Coverage data is based on head (7a60da6) compared to base (cea5b36). Patch has no changes to coverable lines.

:exclamation: Current head 7a60da6 differs from pull request most recent head 3710723. Consider uploading reports for the commit 3710723 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #521      +/-   ##
==========================================
+ Coverage   93.10%   93.14%   +0.03%     
==========================================
  Files           8        8              
  Lines         174      175       +1     
  Branches       16       17       +1     
==========================================
+ Hits          162      163       +1     
  Misses         12       12              

Impacted Files	Coverage Δ
src/package_manager/pip.ts	100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[christophebedard]

Seems fine if it works!

It doesn't :(

[christophebedard]

I was finally able to properly look into the issue. Jobs that run inside Docker fail when running npm run build:

> ncc build src/setup-ros.ts -o dist

ncc: Version 0.36.1
ncc: Compiling file index.js into CJS
ncc: Using [email protected] (local user-provided)
Error: EACCES: permission denied, open 'dist/index.js'
    at Object.openSync (node:fs:590:3)
    at writeFileSync (node:fs:2202:35)
    at handler (/__w/setup-ros/setup-ros/node_modules/@vercel/ncc/dist/ncc/cli.js.cache.js:1:82319) {
  errno: -13,
  syscall: 'open',
  code: 'EACCES',
  path: 'dist/index.js'
}

The dist/index.js file is owned by root:

+ ls -al dist/index.js
-rw-rw-r-- 1 root root 271619 Feb 11 19:21 dist/index.js

If I change the workflow config to run the job directly in the runner (i.e., ubuntu-22.04 runner instead of ubuntu:jammy Docker image), then the file is owned by the runner user:

+ ls -al dist/index.js
-rw-r--r-- 1 runner docker 271619 Feb 11 19:34 dist/index.js

This seems to be the root issue:

• https://github.com/actions/runner/issues/434

And the following issues seem relevant:

• https://github.com/actions/runner/issues/1317
• https://github.com/actions/runner/issues/1282#issuecomment-1063974931

However, we're not building our own image, we're using an official Ubuntu image. Also, I don't know why this is happening when trying to upgrade to Node 16.

[christophebedard]

I think I found the underlying issue. The first failure happens when we run npm run build, where "build" is defined under scripts in the package.json file. This runs ncc build src/setup-ros.ts -o dist. However, it runs it as a different user. See the output below, which was obtained by modifying the build-and-test.sh script. We're root and the dist/index.js belongs to root, but ncc build ... is being run as user with ID=1001 (when we call npm run build), as shown by the id command prepended to the build script/command:

Output

+ whoami
root
+ id
uid=0(root) gid=0(root) groups=0(root)
+ ls -al dist/index.js
-rw-r--r-- 1 root root 271619 Feb 12 17:04 dist/index.js
+ chown -R 1001:1001 /__w
+ ls -al dist/index.js
+ npm ci
-rw-r--r-- 1 1001 1001 271619 Feb 12 17:04 dist/index.js
npm WARN deprecated [email protected]: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.

> [email protected] prepare
> husky install

husky - Git hooks installed

added 447 packages, and audited 448 packages in 11s

77 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
+ npm run build

> [email protected] build
> id && ncc build src/setup-ros.ts -o dist

uid=1001 gid=1001 groups=1001
ncc: Version 0.36.1
ncc: Compiling file index.js into CJS
ncc: Using [email protected] (local user-provided)
265kB  dist/index.js
265kB  [39[15](https://github.com/ros-tooling/setup-ros/actions/runs/4157357676/jobs/7191803601#step:5:16)ms] - ncc 0.36.1
+ ls -al dist/index.js
-rw-r--r-- 1 1001 1001 271619 Feb 12 17:04 dist/index.js

The solution/workaround is therefore to chown everything under /__w (which is the runner's workspace mounted into the Docker container) to user ID=1001 if that directory exists, i.e., if we're inside a Docker container.

This seems to be a weird mix of the Docker issue I mentioned in my previous comment and npm's behaviour: https://github.com/npm/cli/issues/4589. npm used to have an unsafe-perm option to disable this behaviour, but it doesn't exist anymore. Let's go with this workaround for now.

I just need to confirm that this issue doesn't affect end users of setup-ros/action-ros-ci.

[christophebedard]

I just need to confirm that this issue doesn't affect end users of setup-ros/action-ros-ci.

It doesn't affect end users, as shown in this action-ros-ci PR that bumps Node.js to v16 as well: https://github.com/ros-tooling/action-ros-ci/pull/778. This only affects npm run ... commands as part of our CI, so we're good to go!