Skip to content
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

fix: also check for "EISDIR" in `mkdirp` #10545

Open
wants to merge 2 commits into
base: master
from

Conversation

@sodatea
Copy link
Contributor

sodatea commented Mar 12, 2020

Fixes #10544.

Though the EISDIR error code is not mentioned in the POSIX standard,
FreeBSD and memfs both throw this error when trying to run mkdir on
/. So essentially we should treat it the same as EEXIST.

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

Fixes #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
@webpack-bot

This comment has been minimized.

Copy link
Contributor

webpack-bot commented Mar 12, 2020

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)
Copy link
Member

evilebottnawi left a comment

Please add test

@webpack-bot

This comment has been minimized.

Copy link
Contributor

webpack-bot commented Mar 17, 2020

@sodatea Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@evilebottnawi Please review the new changes.


module.exports = {
findBundle: function() {
return [path.relative(outputDirectory, "/tmp/bundle.js")];

This comment has been minimized.

Copy link
@sodatea

sodatea Mar 17, 2020

Author Contributor

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.

@webpack-bot webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Mar 17, 2020
@webpack-bot

This comment has been minimized.

Copy link
Contributor

webpack-bot commented Mar 17, 2020

@sodatea The most important CI builds failed. This way your PR can't be merged.

Please take a look at the CI results from travis (success) and appveyor (failure) and fix these issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.