Remove rmDir references #840
Conversation
aeisenberg
force-pushed
the
aeisenberg/fix-rm
branch
2 times, most recently
from
9080691 to
82f7736
Compare
`rmDir` is not available on the node version used by the actions runner. Instead, use the `del` package. It is safe, well-tested, and cross-platform.
aeisenberg
force-pushed
the
aeisenberg/fix-rm
branch
from
82f7736 to
45dc27d
Compare
| @@ -268,8 +275,8 @@ function createToolPath( | |||
| ); | |||
| logger.debug(`destination ${folderPath}`); | |||
| const markerPath = `${folderPath}.complete`; | |||
| fs.rmSync(folderPath, { recursive: true, force: true }); | |||
There was a problem hiding this comment.
Could this be fs.rmdirSync rather than relying on a new dependency? Or does that not offer the same recursive/force behaviour?
There was a problem hiding this comment.
rmDirSync was also newly added in v14. So, can't use that method on an actions runner.
There was a problem hiding this comment.
https://nodejs.org/api/fs.html#fsrmdirsyncpath-options claims it was added in 12 (though various options are deprecated in 14 and 16, which might be annoying when we upgrade).
There was a problem hiding this comment.
Ah...so it is.
Yes, looks like the recursive option was deprecated. We would need to add it now, and then remove it later when the node version increased.
I'm still thinking it is cleaner to use del. It's a very popular package and handles all of these edge cases correctly.
edoardopirovano
left a comment
edoardopirovano
left a comment
There was a problem hiding this comment.
LGTM. I think we may as well use del wherever possible now that we've introduced the (very reasonable) dependency.
|
Looks like we need a |
The dependency already existed. It's just that it was transitive. I had to make it a top level dependency in order to be allowed to import it. |
Curious how this check passed: https://github.com/github/codeql-action/runs/4462419683?check_suite_focus=true |
Ah, even better :) |
|
I think builds on |
rmDiris not available on the node version used by the actions runner.Instead, use the
delpackage. It is safe, well-tested, andcross-platform. Also, downgrade the
@types/nodepackage so that it more closely reflects what is being run on the server.This should fix the failing tests on main.
Interestingly, there are several other places in the code where
rmDiris being used, but it is not causing problems because (I think) these code paths are only hit by the runner.Merge / deployment checklist