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

Revert "fs: add a temporary fix for re-evaluation support" #6413

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@jasnell
Member

jasnell commented Apr 27, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

fs

Description of change

As planned, This reverts commit 1d79787.

Fixes: #5213
Refs: #5102

@nodejs/ctc

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Apr 27, 2016

LGTM

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Apr 27, 2016

LGTM.

@JungMinu

This comment has been minimized.

Member

JungMinu commented Apr 27, 2016

LGTM

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Apr 27, 2016

Has anyone tried running the npm in node master with this?

@Fishrock123 Fishrock123 added this to the 7.0.0 milestone Apr 27, 2016

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Apr 27, 2016

make test-npm passes on master with this. All clear.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Apr 27, 2016

@Fishrock123 eslint should also work, as it uses a newer version of graceful-fs.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 27, 2016

@Fishrock123 ... yeah, I had tested it last night right before opening the PR. Everything appeared to be ok but if there's any doubt there's no harm in leaving this PR open for a bit until we're sure. I just didn't want it to go un-done.

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 27, 2016

Also, I marked this semver-major defensively. It probably does not need to be semver-major but it definitely likely should be don't land on v6 and below.

@ChALkeR ChALkeR referenced this pull request Apr 28, 2016

Closed

[epic] vinyl-fs version 3.0.0 #1604

15 of 15 tasks complete
@jasnell

This comment has been minimized.

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Apr 30, 2016

It probably does not need to be semver-major but it definitely likely should be don't land on v6 and below.

I agree, let's just use the other labels, unless we really think it should be in a v7 (probably more like LTS 3/ v8) changelog.

@jasnell

This comment has been minimized.

Member

jasnell commented May 16, 2016

@nodejs/ctc ... are we ready to land this or no?

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 16, 2016

I'm a little hesitant, maybe for v8?

@jasnell

This comment has been minimized.

Member

jasnell commented May 17, 2016

I'm definitely -1 on waiting until v8. The original decision was to revert this in v7. I have no issues letting this sit a while longer but definitely don't want to push it out that far (largely because there's simply no reason to)

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented May 17, 2016

@jasnell I will try to create a list of packages currently affected by this.

@rvagg

This comment has been minimized.

Member

rvagg commented May 17, 2016

lgtm but don't see reason to hurry landing it to introduce code churn making cherry-picking harder

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented May 17, 2016

@rvagg One reason for landing this sooner than later would be so that people who test nightlies and ignored the deprecation message will actually notice things being broken if something still uses old graceful-fs versions.

@jasnell

This comment has been minimized.

Member

jasnell commented May 17, 2016

The cherry-picking impact of this should be minimal and fairly easy to deal
with.
On May 17, 2016 5:37 AM, "Rod Vagg" notifications@github.com wrote:

lgtm but don't see reason to hurry landing it to introduce code churn
making cherry-picking harder


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6413 (comment)

@ChALkeR ChALkeR referenced this pull request May 18, 2016

Closed

fs: move SyncWriteStream to internal/fs #6749

0 of 3 tasks complete
@jasnell

This comment has been minimized.

Member

jasnell commented Aug 25, 2016

I'm +1 for unblocking but I'd rather land #8166 than this one.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 25, 2016

@jasnell I believe we could do both, but the one that gets landed later should get manually rebased.
I don't see any reason for blocking any of those against each other.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 25, 2016

Well, the other PR also reverts this but in a different way. If that one landed, there'd be no reason to also land this one.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 25, 2016

@jasnell No, that one keeps it as a deprecation warning, while the idea here is to remove the warning and turn it into a throw.

We could either:

  1. Land #8166, then make it a throw instead of a warning in a rebase of this PR.
  2. Land this (turn warning into a throw), then rebase #8166 in a way so that it keeps the throw (instead of keeping the warning).
@jasnell

This comment has been minimized.

Member

jasnell commented Aug 25, 2016

Ah.. I see what you're saying. I can update #8166 to make it a throw if that's what we want it to do.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 25, 2016

@jasnell only if this happens to land first, becase that is a semver-major change that is unrelated to #8166. 😉

So, can we unblock this?

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 26, 2016

/cc @nodejs/ctc.

I am pretty sure this was blocked by gulp and graceful-fs@3 not being compatible. They are fine now.

I propose to unblock and land this for v7.0, we already have a bunch of other PRs blocked by this: #6749, #6573, #7162, #2025, #8277#8292, and most part of those are active.

See current usage data in #6413 (comment).

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 26, 2016

Unblocking should be fine

@ChALkeR ChALkeR removed the blocked label Aug 26, 2016

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 26, 2016

Once again: LGTM =).

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 26, 2016

@nodejs/ctc ... because graceful-fs v3 has now been updated to avoid this issue, I'd like to go ahead and land this revert. Any objections?

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Aug 26, 2016

lgtm

On Fri, Aug 26, 2016, 5:59 PM James M Snell notifications@github.com
wrote:

@nodejs/ctc https://github.com/orgs/nodejs/teams/ctc ... because
graceful-fs v3 has now been updated to avoid this issue, I'd like to go
ahead and land this revert. Any objections?


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

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 27, 2016

@nodejs/ctc ... if there are no objections, I will land this on Monday.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Aug 27, 2016

LGTM

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Aug 27, 2016

Still LGTM.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 29, 2016

New CI before landing: https://ci.nodejs.org/job/node-test-pull-request/3874/

Red in the last run, seemingly unrelated but just in case: https://ci.nodejs.org/job/node-test-pull-request/3875/

jasnell added a commit that referenced this pull request Aug 29, 2016

Revert "fs: add a temporary fix for re-evaluation support"
As planned, This reverts commit 1d79787.

Fixes: #5213
PR-URL: #6413
Reviewed-By: Ben Noordhuis <info@noordhuis.nl>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: JungMinu - Minwoo Jung <jmwsoft@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell

This comment has been minimized.

Member

jasnell commented Aug 29, 2016

Landed (finally) in 49ef3ae

@jasnell jasnell closed this Aug 29, 2016

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 29, 2016

At long last =)

@jasnell, thanks for taking care of this!

jasnell added a commit to jasnell/node that referenced this pull request Sep 2, 2016

fs: use process.emitWarning to print deprecation warning
As an alternative to nodejs#6413, use
process.emitWarning() instead of the internal printDeprecationMessage
in order to avoid use of an internal only API.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Sep 23, 2016

fs: move stringToFlags() to lib/internal
PR-URL: nodejs#7162
Refs: nodejs#6413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

jasnell added a commit that referenced this pull request Oct 10, 2016

fs: move stringToFlags() to lib/internal
PR-URL: #7162
Refs: #6413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Tyriar added a commit to Tyriar/gulp-untar that referenced this pull request Oct 23, 2016

Uplevel tar dep to v1
This upgrades the transitive dep graceful-fs@3 to v4 which gets around the
breakage caused by: nodejs/node#6413

@Tyriar Tyriar referenced this pull request Oct 23, 2016

Merged

Uplevel tar dep to v1 #7

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment