Remove rmDir references - #840
adityasharad merged 1 commit into
Conversation
Sorry, something went wrong.
`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.
| @@ -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?
Sorry, something went wrong.
There was a problem hiding this comment.
rmDirSync was also newly added in v14. So, can't use that method on an actions runner.
Sorry, something went wrong.
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).
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
Looks like we need a force flag to delete outside the current directory: https://github.com/github/codeql-action/runs/4464110110?check_suite_focus=true#step:7:4172 |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Curious how this check passed: https://github.com/github/codeql-action/runs/4462419683?check_suite_focus=true |
Sorry, something went wrong.
Ah, even better :) |
Sorry, something went wrong.
|
I think builds on main have additional data to clean up (the database that's built and uploaded for remote queries). |
Sorry, something went wrong.
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. Also, downgrade the @types/node package 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 rmDir is being used, but it is not causing problems because (I think) these code paths are only hit by the runner.
Merge / deployment checklist