repl: runtime deprecate NODE_REPL_HISTORY_FILE by maclover7 · Pull Request #16505 · nodejs/node · GitHub

Refs: #13876

Just a note that I've been trying to add tests for this, but it's been
difficult because it seems like most REPL tests directly patch into
internals, and are not run via a child process.

Also, not sure if this should even be landing, since it's very close to the v9 release candidate release... cc @jasnell

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

repl

It doesn't seem correct to me. Who calls the function returned by deprecate()?

@lpinca sorry, I didn't read internal/util source correctly, I thought deprecate auto-printed the deprecation. updated my pr.

I think this should be called in setupHistory() iff oldHistoryPath is not null or undefined. By calling it here, it won't be displayed for users who have set the NODE_REPL_HISTORY_FILE env variable, but opening or parsing that history file throws an Error.

Hmm, fair point. But wouldn't we want to only show the error message to NODE_REPL_HISTORY_FILEs that actually parse correctly? If the file is broken, they probably wouldn't be able to convert it to the new file format anyway, right? 😬

Hm - ping @lance @maclover7 ?

My guess would be that there's nobody left using the old format anyway by now and that this particular difference would really not matter anyway

Sorry for my latency. @addaleax @maclover7 your points are valid. TBH, I think it would be more correct if moved to setupHistory(), but I won't block the PR on that.

Ping @maclover7

CI: https://ci.nodejs.org/job/node-test-pull-request/11827/

CI: https://ci.nodejs.org/job/node-test-pull-request/11829/

Might it be more clear to say "use NODE_REPL_HISTORY instead"? That's what the docs say.

LGTM with a suggestion

Labeled semver-major due to policy that the introduction of a runtime deprecation is treated as a breaking change.

This needs one more TSC approval if it is to land. @nodejs/tsc

nit: it doesn't seem necessary to pass a value to the function

updated @targos @Trott

nit: extraneous space after the arrow

removed extra space, good catch @targos

CI: https://ci.nodejs.org/job/node-test-pull-request/11871/

Seems like on windows the test has some issues? https://ci.nodejs.org/job/node-test-binary-windows/13342/COMPILED_BY=vcbt2015,RUNNER=win10,RUN_SUBSET=1/console

cc @nodejs/platform-windows trying to debug this locally, it looks like per the TAP extended results the deprecation warning is being emitted (is visible in the tap stack), but for some reason the common.hijackStderr is not being called -- is this a windows specific thing?

Ping @nodejs/platform-windows PTAL

The common.hijakStderr works as expected, deprecation message you see in the TAP results is from one of the tests that is missing deprecated: true:
https://github.com/maclover7/node/blob/2476814b0a315655d3eac33cd6eb6f4a710a9510/test/parallel/test-repl-persistent-history.js#L130-L134

The test is failing because of this test-case:
https://github.com/maclover7/node/blob/2476814b0a315655d3eac33cd6eb6f4a710a9510/test/parallel/test-repl-persistent-history.js#L177-L183

For whatever reason data buffer passed to hijakStderr callback is empty.

Refs: #13876 Just a note that I've been trying to add tests for this, but it's been difficult because it seems like most REPL tests directly patch into internals, and are not run via a child process.

CI: https://ci.nodejs.org/job/node-test-pull-request/12365/

LGTM

@bzoz Tried to remediate, ran another CI, and Windows is still failing. Can you think of why an empty data buffer is being passed from common.hijackStderr -- would appreciate any leads. Definitely a Windows specific issue at this point.

I investigated this, I did not diagnose this correctly the first time around, it looks like this:

This fails only on Windows because only on that platform this test uses execSync to call ATTRIB +H to set make file hidden. This will in turn always write something to stderr, even if it is an empty buffer. In the hijackStderr callback if (data) { ... } will evaluate to true (even if the data is an empty buffer) and this will make the assert in that test fail. With {stdio: 'ignore'} option added to the execSync call, nothing will be written to stderr and the test will pass on Windows. But, it will also pass on the current master.

The hijakStderr callback needs something like common.mustCall, e.g.:

  if (deprecated) {
    common.hijackStderr(common.mustCall((data) => {
      assert.ok(deprecated && depMsg.test(data));
    }));
  }

As you observed in the TAP results, the deprecation message was only displayed once. With the test like this, it will fail for all but the first "deprecated: true" test cases, for which no deprecated message is shown. This will happen on both Windows and Linux.

@bzoz fwiw (and I don't know if this will add anything of value to the conversation), but I mucked around with the history file quite a bit about a year and a half ago. There is a lot of discussion about those changes here. This #7005 (comment) and the few comments below it might shed a little light. At the moment I don't have the bandwidth to dig deeper - just wanted to point out that there has been some work around some of these Windows permissions issues in the past.

To me it looks like the hijack is called multiple times and will therefore also trigger for tests that have nothing to do with the current test.
So I think the hijack should be deactivated as soon as it is not required anymore.

In that case the inner deprecated check should also be obsolete.

@maclover7 would you be so kind and have another look?

@bzoz I am not sure how to follow your example code in combination with your comment. How should a common.mustCall fix the issue? As far as I understand it the error is coming from inside that function, so it is called anyway. So adding the mustCall should only verify that it was indeed triggered properly.

Ping @maclover7

To make the test pass on Windows, add {stdio: 'ignore'} to the execSync call.

However, this test is broken, since it does not check if the deprecation message was shown at all. This is why common.mustCall should be used in hijakStderr. Otherwise, this test will also pass with the current master.

Closing due to long inactivity. To be frank: I personally feel like in this very seldom case it is possible to remove that environment variable without a proper deprecation cycle. The reason is not only that no one has probably really used it for more than a few days but also that I do not see how removing would really do anyone harm. Relying on the repl history file should be very rare...

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Oct 25, 2017

lpinca reviewed Oct 26, 2017

View reviewed changes

lance suggested changes Nov 3, 2017

View reviewed changes

lance approved these changes Nov 30, 2017

View reviewed changes

BridgeAR approved these changes Nov 30, 2017

View reviewed changes

Trott reviewed Dec 4, 2017

View reviewed changes

Trott approved these changes Dec 4, 2017

View reviewed changes

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 4, 2017

targos approved these changes Dec 4, 2017

View reviewed changes

targos reviewed Dec 4, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

maclover7 added 6 commits January 1, 2018 20:25

repl: runtime deprecate NODE_REPL_HISTORY_FILE

ad7c486

Refs: #13876 Just a note that I've been trying to add tests for this, but it's been difficult because it seems like most REPL tests directly patch into internals, and are not run via a child process.

[squash] move dep to separate function, add tests

a133000

fix lint error

4f3c85a

fix nits

d17c56a

remove extra space

da8d65b

squash

9a8cf0d

mcollina approved these changes Jan 2, 2018

View reviewed changes

BridgeAR added the wip Issues and PRs that are still a work in progress. label Jan 5, 2018

BridgeAR reviewed Jan 19, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

jasnell approved these changes Jan 31, 2018

View reviewed changes

bzoz suggested changes Feb 1, 2018

View reviewed changes

BridgeAR closed this Feb 7, 2018

repl: runtime deprecate NODE_REPL_HISTORY_FILE by maclover7 · Pull Request #16505 · nodejs/node · GitHub

Uh oh!

Uh oh!

repl: runtime deprecate NODE_REPL_HISTORY_FILE - #16505

repl: runtime deprecate NODE_REPL_HISTORY_FILE#16505maclover7 wants to merge 6 commits intonodejs:masternodejs/node:masterfrom maclover7:jm-dep-historyCopy head branch name to clipboard

Conversation

maclover7 commented Oct 25, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Nov 23, 2017

Uh oh!

maclover7 commented Nov 30, 2017

Uh oh!

maclover7 commented Nov 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 4, 2017

Uh oh!

Trott commented Dec 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maclover7 commented Dec 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maclover7 commented Dec 4, 2017

Uh oh!

BridgeAR commented Dec 6, 2017

Uh oh!

maclover7 commented Dec 11, 2017

Uh oh!

BridgeAR commented Dec 28, 2017

Uh oh!

bzoz commented Dec 29, 2017

Uh oh!

maclover7 commented Jan 2, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

maclover7 commented Jan 2, 2018

Uh oh!

bzoz commented Jan 4, 2018

Uh oh!

lance commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jan 19, 2018

Uh oh!

BridgeAR commented Jan 31, 2018

Uh oh!

bzoz left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

repl: runtime deprecate NODE_REPL_HISTORY_FILE#16505
maclover7 wants to merge 6 commits into
nodejs:masterfrom
maclover7:jm-dep-history