FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Revert fs changes by MylesBorins · Pull Request #7846 · nodejs/node · GitHub

/ node Public

Revert fs changes - #7846

Closed
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:revert-fs-changes
ConversationCommits (1)ChecksFiles changed
Closed

Revert fs changes#7846
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:revert-fs-changes

Conversation

MylesBorins commented Jul 22, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

Description of change

9359de9 and c86c1ee are two semver major changes to the fs module that landed in the last 24 hours. When trying to run CITGM it was discovered that npm set was throwing an error

0 info it worked if it ends with ok
1 verbose cli [ '/usr/local/bin/node',
1 verbose cli   '/usr/local/bin/npm',
1 verbose cli   'set',
1 verbose cli   'progress=false' ]
2 info using npm@3.10.3
3 info using node@v7.0.0-pre
4 info config set "progress" "false"
5 verbose stack TypeError: cb is not a function
5 verbose stack     at /usr/local/lib/node_modules/npm/node_modules/graceful-fs/polyfills.js:210:7
5 verbose stack     at FSReqWrap.oncomplete (fs.js:103:15)
6 verbose cwd /Users/thealphanerd/code/node/master
7 error Darwin 14.5.0
8 error argv "/usr/local/bin/node" "/usr/local/bin/npm" "set" "progress=false"
9 error node v7.0.0-pre
10 error npm  v3.10.3
11 error cb is not a function
12 error If you need help, you may report this error at:
12 error     <https://github.com/npm/npm/issues>
13 verbose exit [ 1, true ]

I think that we should revert asap and revisit these two commits in a new PR to figure out what is going on here

edit: it looks like the problem may be in graceful-fs. I'm going to dig into finding a fix for it, I will also look into adding it to citgm, as it has definitely been a module that has caused pain in the past

MylesBorins added confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. labels Jul 22, 2016
nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 22, 2016

MylesBorins commented Jul 22, 2016

Copy link
Copy Markdown
Contributor Author

/cc @thefourtheye @trevnorris

cjihrig mentioned this pull request Jul 23, 2016
Closed
2 tasks
Trott mentioned this pull request Jul 23, 2016
Closed
thefourtheye mentioned this pull request Jul 23, 2016
Merged
Trott mentioned this pull request Jul 23, 2016
Closed
2 tasks

trevnorris commented Jul 24, 2016

Copy link
Copy Markdown
Contributor

@thealphanerd: i've gotten bored being tied down by a dependency of a dependency. graceful-fs reverted the proper fix that would have prevented this b/c of wanting ancient node support. though iirc npm no longer supports that version of node, so the fix can land yet again.

it's pathetic these need to be reverted. let's prioritize fixing our broken dependency then landing these improvements again.

ChALkeR commented Jul 24, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

graceful-fs reverted the proper fix that would have prevented this b/c of wanting ancient node support.

…

let's prioritize fixing our broken dependency then landing these improvements again.

Uh-oh. I hope this won't result in a major graceful-fs version bump to fix this.

MylesBorins mentioned this pull request Jul 25, 2016
Closed
4 tasks

jasnell commented Jul 25, 2016

Copy link
Copy Markdown
Member

to be honest I'm not that thrilled by this either. Let's get this on the @nodejs/ctc agenda for discussion

MylesBorins commented Jul 26, 2016

Copy link
Copy Markdown
Contributor Author

So I did a bit more digging today.

There was indeed an issue in graceful-fs regarding how chown was being polyfilled, there is currently a PR in to fix this

Even with this fix patched onto npm we are still encountering failures due to other modules in the graceful-fs dep tree that make async fs calls without providing a callback. Due to the way graceful-fs shims chmod, the stack traces are not super useful. It has been suggested in the thread that there may be a more graceful way to handle that situation, by providing a no-op callback to chown. This PR will likely fix the pain point in graceful-fs, but it may take a bit of time.

While it may be easy to consider this "another graceful-fs" problem I think it is a sign of wider usage of async functions without a callback in the ecosystem. While doing so is obviously not a best practice, it is happening in the wild, and this change is breaking stuff.

@ChALkeR and I were discussing this on IRC and are going to attempt to figure out a list of modules that will break with this change, and will be working on getting updates across the ecosystem to make this change less disruptive.

We should revert until the fix to graceful-fs lands, the fix is downstreamed to npm, and we have made a decent effort to make sure the ecosystem breakages are minimal.

ChALkeR commented Jul 26, 2016

Copy link
Copy Markdown
Member

@jasnell, @trevnorris This isn't only graceful-fs fault this time. Even with graceful-fs being patched, there are still errors — it looks like 9359de9 breaks too many modules. Will post the actual results a bit later.

jasnell commented Jul 26, 2016

Copy link
Copy Markdown
Member

Yep, understood. I had a chat with @thealphanerd about it earlier this
evening. I'm +1 on reverting until it's sorted out more

On Monday, July 25, 2016, Сковорода Никита Андреевич <
notifications@github.com> wrote:

@jasnell https://github.com/jasnell, @trevnorris
https://github.com/trevnorris This isn't only graceful-fs fault this
time. Even with graceful-fs being patched, there are still errors — it
looks like 9359de9
9359de9
breaks too many modules. Will post the actual results a bit later.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7846 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eVPeIJQ0k60tmcsLvOxyXRNSTlHcks5qZZpCgaJpZM4JTOGq
.

evanlucas commented Jul 26, 2016

Copy link
Copy Markdown
Contributor

+1 on reverting. I think the realpath situation has shown us we really need to be careful with the impact that breaking changes have on the ecosystem

Trott mentioned this pull request Jul 26, 2016
Closed

MylesBorins commented Jul 26, 2016

Copy link
Copy Markdown
Contributor Author

For reference isaacs/node-graceful-fs#71 has landed in graceful-fs

thefourtheye added a commit to thefourtheye/npm that referenced this pull request Jul 27, 2016
graceful-fs@4.1.5
45164cc 
graceful-fs had a bug fix, isaacs/node-graceful-fs#71,
which would fix the problem in Node.js nodejs/node#7846
thefourtheye mentioned this pull request Jul 27, 2016
Closed

bnoordhuis commented Jul 27, 2016

Copy link
Copy Markdown
Member

These changes would be released in v7, correct? That is a major release and therefore allowed to be backwards incompatible.

While we should not be introducing backwards incompatible changes just for the sake of it, code that omits the callback is borderline broken and I think it's good to turn that into an early error. It's one less avenue for time bombs to sneak into a code base.

We shouldn't be afraid to land changes that expose bugs, provided there is an easy way to fix broken code, even when that causes some initial breakage and churn in the ecosystem. Churn because of bugs fixed is good churn.

ChALkeR commented Jul 27, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

@bnoordhuis It should still go through a proper deprecation cycle. Printing a warning at first would be better, I suppose.

ChALkeR commented Jul 27, 2016

Copy link
Copy Markdown
Member

Could we print warnings for now and turn those into errors in v8.0?

thefourtheye mentioned this pull request Jul 27, 2016
Closed
2 tasks
bzoz mentioned this pull request Jul 27, 2016
Closed
2 tasks

jasnell commented Jul 27, 2016

Copy link
Copy Markdown
Member

Can we please split this into two separate PRs, one for each revert commit. /cc @nodejs/ctc

MylesBorins commented Jul 27, 2016

Copy link
Copy Markdown
Contributor Author

@jasnell the original commit cannot be reverted cleanly without the other commit being reverted first

isaacs commented Jul 28, 2016

Copy link
Copy Markdown
Contributor

This project has a process for making planned breaking changes to core features.

It starts with documentation and deprecation warnings, and then the breaking change comes later.

Introducing this sort of thing without a warning to the user is irresponsible. Please do not do that.

I understand that it's frustrating to work within the constraints imposed by not breaking users' programs. It's frustrating to be responsible sometimes. There are lots of projects out there that don't require this level of responsibility because they aren't popular platforms, but that's what's required to do a good job with this one.

These processes are in place to protect the stability of the platform because stability is what matters most for a platform.

addaleax commented Aug 3, 2016

Copy link
Copy Markdown
Member

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/344/

(I think that’s what was requested on the CTC meeting, and it won’t hurt anyway.)

jasnell commented Aug 3, 2016

Copy link
Copy Markdown
Member

Woo! saved me a step, I was just heading off to do exactly that...

Revert "fs: make callback mandatory to all async functions"
94e7af6 
This reverts commit 9359de9.

Original Commit Message:

    The "fs" module has two functions called `maybeCallback` and
    `makeCallback`, as of now.

    The `maybeCallback` creates a default function to report errors, if the
    parameter passed is not a function object. Basically, if the callback
    is omitted in some cases, this function is used to create a default
    callback function.

    The `makeCallback`, OTOH, creates a default function only if the
    parameter passed is `undefined`, and if it is not a function object it
    will throw an `Error`.

    This patch removes the `maybeCallback` function and makes the callback
    function argument mandatory for all the async functions.

    PR-URL: nodejs#7168
    Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins closed this Aug 4, 2016
MylesBorins reopened this Aug 4, 2016

MylesBorins commented Aug 4, 2016

Copy link
Copy Markdown
Contributor Author

so there are failures in citgm... especially for

  • request
  • async
  • vinyl-fs
  • eslint

All of eslint failures are infra related, they are using a module that doesn't support node v7. Async / request has been known flaky for a while, we just have not been seeing the errors due to a bug in citgm. I'll be digging more into them next week. vinyl-fs has had some problems this past week at all.

When I get back next week I'll do a deep dive across all versions and update the flaky status for all modules for all lines

MylesBorins commented Aug 4, 2016

Copy link
Copy Markdown
Contributor Author

Can people throw some LGTM's on this? Without this reverted npm is broken on master (has been for almost 2 weeks now)

addaleax commented Aug 4, 2016

Copy link
Copy Markdown
Member

Ping @nodejs/ctc
(Mine still stands, fwiw)

jasnell commented Aug 4, 2016

Copy link
Copy Markdown
Member

The CTC decision yesterday was to go ahead with the revert so... LGTM.

The bias, however, was towards finding a way for the revert to be temporary. Specifically, the overall opinion appears to be that we want these APIs to throw if a callback is not provided, but we'll just take a bit more time to get there. Emitting either a deprecation warning or process warning (process.emitWarning()) seemed to be the best approach (I think we landed on deprecation warning yesterday but I'm not 100% sure)

addaleax commented Aug 4, 2016

Copy link
Copy Markdown
Member

Landed in 21b0a27

addaleax closed this Aug 4, 2016
addaleax pushed a commit that referenced this pull request Aug 4, 2016
Revert "fs: make callback mandatory to all async functions"
21b0a27 
This reverts commit 9359de9.

Original Commit Message:

    The "fs" module has two functions called `maybeCallback` and
    `makeCallback`, as of now.

    The `maybeCallback` creates a default function to report errors, if the
    parameter passed is not a function object. Basically, if the callback
    is omitted in some cases, this function is used to create a default
    callback function.

    The `makeCallback`, OTOH, creates a default function only if the
    parameter passed is `undefined`, and if it is not a function object it
    will throw an `Error`.

    This patch removes the `maybeCallback` function and makes the callback
    function argument mandatory for all the async functions.

    PR-URL: #7168
    Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: #7846
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig mentioned this pull request Aug 8, 2016
Closed

cjihrig commented Aug 10, 2016

Copy link
Copy Markdown
Contributor

Adding the don't land on v6.x label, as this reverts a semver major change.

jasnell commented Aug 10, 2016

Copy link
Copy Markdown
Member

@cjihrig ... I believe reverting these in v6 is something that we need to do and was discussed on the @nodejs/ctc call. Specifically, the plan is to back off throwing when the callback is not given and to issue a deprecation warning instead. Throwing just caused too much breakage in the ecosystem.

@nodejs/ctc @thealphanerd ... does this match your recollection?

cjihrig commented Aug 10, 2016

Copy link
Copy Markdown
Contributor

@jasnell this reverts PR #7168, which is semver major, and doesn't appear to have landed on v6. So, I think this only needed to be reverted in master.

jasnell commented Aug 10, 2016

Copy link
Copy Markdown
Member

-.- ... heh, you're right, of course. I keep thinking that this one had landed in v6 but I keep confusing it with the other fs changes.

cjihrig commented Aug 10, 2016

Copy link
Copy Markdown
Contributor

Yea, fs is a bit of a mess right now :-)

iarna added a commit to npm/npm that referenced this pull request Aug 11, 2016
graceful-fs@4.1.5
51d49d2 
Fix bug in chmod wrapper that was tickled by recent node.js changes.
(nodejs/node#7846)

PR-URL: isaacs/node-graceful-fs#71
Credit: @thefourtheye
Reviewed-By: @isaacs
iarna pushed a commit to npm/npm that referenced this pull request Aug 11, 2016
graceful-fs@4.1.5
7e0cdff 
graceful-fs had a bug fix, isaacs/node-graceful-fs#71,
which would fix the problem in Node.js nodejs/node#7846

PR-URL: #13497
Credit: @thefourtheye
Reviewed-By: @iarna
zkat mentioned this pull request Aug 12, 2016
Closed
13 tasks
zkat mentioned this pull request Sep 22, 2016
Closed
4 tasks
Trott removed the ctc-agenda label Oct 20, 2016
gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
MylesBorins deleted the revert-fs-changes branch November 14, 2017 17:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL