fix: should not fail when output directory is the root directory #10545
Conversation
Fixes webpack#10544. Though the `EISDIR` error code is not mentioned in the POSIX standard, FreeBSD and memfs both throws this error when trying to run `mkdir` on `/`. So essentially we should treat it the same as `EEXIST`. References: * https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=59739 * streamich/memfs#326
|
For maintainers only:
|
webpack-bot
added
PR: review-outdated
and removed
PR: reviewed-changes-requested
labels
|
|
||
| module.exports = { | ||
| findBundle: function() { | ||
| return [path.relative(outputDirectory, "/tmp/bundle.js")]; |
There was a problem hiding this comment.
If I return an absolute path here directly, the _require function in ConfigTestCases.test.js would require the bundle directly, making it lack of essential context utilities like the it function.
So the only workaround I found is to calculate the relative path of /tmp/bundle.js.
|
/cc @sodatea friendly ping |
|
I think I need to work out a more generic way to deal with the root directory case, instead of relying on the error code because different OSes😅 |
sodatea
changed the title
mkdirp
lib/util/fs.js
Outdated
| @@ -115,6 +115,11 @@ exports.dirname = dirname; | |||
| * @returns {void} | |||
| */ | |||
| const mkdirp = (fs, p, callback) => { | |||
| if (path.resolve(p) === path.resolve("/")) { | |||
There was a problem hiding this comment.
Do you really need so many path.resolve? They have a performance cost. I think p is always a absolute path anyway. What is about windows absolute paths?
| if (path.resolve(p) === path.resolve("/")) { | |
| if (/^(\/|[A-Za-z]:\\?|\\\\[^\\]+\\?)$/.test(p)) { |
There was a problem hiding this comment.
This will fail edge cases like multiple trailing backslashes (such as C:\\\\).
Though, we can avoid the performance overhead by moving the logic into the error handling part.
webpack-bot
added
PR: reviewed-changes-requested
and removed
PR: review-outdated
labels
webpack-bot
added
PR: review-outdated
and removed
PR: reviewed-changes-requested
labels
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
Fixes #10544.
Though the
EISDIRerror code is not mentioned in the POSIX standard,FreeBSD and
memfsboth throw this error when trying to runmkdiron/. So essentially we should treat it the same asEEXIST.References:
What kind of change does this PR introduce?
A bugfix
Did you add tests for your changes?
I'm not sure how I should add tests, because this issue only happens on BSD systems.
Does this PR introduce a breaking change?
No
What needs to be documented once your changes are merged?
Nothing