Fixes: https://github.com/nodejs/node/issues/29975 #29976

aug2uag · 2019-10-14T21:14:04Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Trott · 2019-10-14T21:19:00Z

Welcome @aug2uag and thanks for the pull request! A more descriptive commit message would be welcome. Maybe something like this?

src: remove unnecessary header file

src/node_stat_watcher.cc imports `env.h` but it is not necessary to do so.

Fixes: https://github.com/nodejs/node/issues/29975

(Bonus points for adding an explanation as to why it's not necessary to import it!)

Trott · 2019-10-14T21:19:06Z

@joyeecheung

joyeecheung · 2019-10-14T21:26:10Z

This file uses Environment::GetCurrent which needs env-inl.h, which is transitively included by other headers here. I am not sure removing env.h is an improvement or not.

cc @addaleax @bnoordhuis @danbev any opinions on this?

aug2uag · 2019-10-14T21:27:29Z

Thank you @Trott ! I'm sorry I didn't realize the consequences of no comment. I'm very new to contributing and your comment is very helpful to me.

bnoordhuis · 2019-10-14T22:15:49Z

Instead of removing env.h it should probably be changed to include env-inl.h, that's the predominant pattern in our code base.

src/module_wrap.cc and src/node_report_module.cc could also be updated.

nodejs-github-bot · 2019-10-15T01:11:24Z

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

nodejs-github-bot · 2019-10-15T02:25:52Z

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

nodejs-github-bot · 2019-10-15T02:34:18Z

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

nodejs-github-bot · 2019-10-15T02:47:09Z

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

nodejs-github-bot · 2019-10-15T02:53:40Z

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

nodejs-github-bot · 2019-10-15T03:05:02Z

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

nodejs-github-bot · 2019-10-15T03:58:41Z

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

Trott · 2019-10-15T07:21:45Z

Getting a clean CI on this will probably require landing #29979 first.

aug2uag · 2019-10-15T08:47:24Z

is that an IE render issue? doctool/test-doctool-html.js may have some issues with the HTML or mismatch issue with MD. test-doctool-html.js:100 doesn't close </a></li> and line 65 has breaking HTML:

<h1>
      Sample Markdown with YAML info
      <span>
            <a class="mark" href="#foo_sample_markdown_with_yaml_info" id="foo_sample_markdown_with_yaml_info">#</a>
      </span>
</h1>
<h2>
      Foobar
      <span>
            <a class="mark" href="#foo_foobar" id="foo_foobar">#</a>
      </span>
</h2>
<div class="api_metadata">
      <span>Added in: v1.0.0</span></div>
      <p>Describe <code>Foobar</code> in more detail here.</p>
      <h2>
            Foobar II
            <span>
                  <a class="mark" href="#foo_foobar_ii" id="foo_foobar_ii">#</a>
            </span>
      </h2>
      <div class="api_metadata">
          <details class="changelog">
              <summary>History</summary>
              <table>
                  <tbody>
                      <tr>
                          <th>Version</th>
                          <th>Changes</th>
                      </tr>
                      <tr>
                          <td>v5.3.0, v4.2.0</td>
                          <td>
                              <p><span>Added in: v5.3.0, v4.2.0</span></p>
                          </td>
                      </tr>
                      <tr>
                          <td>v4.2.0</td>
                          <td>
                              <p>The <code>error</code> parameter can now bean arrow function.</p>
                          </td>
                      </tr>
                  </tbody>
              </table>
          </details>
      </div>
      <p>Describe <code>Foobar II</code> in more detail here.<a href="http://man7.org/linux/man-pages/man1/fg.1.html"><code>fg(1)</code></a></p>
      <h2>Deprecated thingy<span><a class="mark" href="#foo_deprecated_thingy" id="foo_deprecated_thingy">#</a></span></h2>
      <div class="api_metadata"><span>Added in: v1.0.0</span><span>Deprecated since: v2.0.0</span></div>
      <p>Describe <code>Deprecated thingy</code> in more detail here.<a href="http://man7.org/linux/man-pages/man1/fg.1p.html"><code>fg(1p)</code></a></p>
      <h2>Something<span><a class="mark" href="#foo_something" id="foo_something">#</a></span></h2>
      <!-- This is not a metadata comment -->
      <p>Describe <code>Something</code> in more detail here. </p>

aug2uag · 2019-10-15T08:55:35Z

Apologies, I wasn't intending to push the changes to lib/_stream_writable.js and I don't know how to revert my changes.

Trott · 2019-10-15T20:29:37Z

@aug2uag The problem likely is that you are opening this pull request against your own master branch. Create a different branch and open a PR against that branch so you can keep your PR changes separate from each other.

aug2uag · 2019-10-15T21:34:42Z

the breaking html was necessary in doctest, sorry about that

this PR should be Ok minus the first commit message i was unable to re-amend

aug2uag · 2019-10-16T16:02:49Z

@Trott is the CI on this broken from the commit message or something else?

nodejs-github-bot · 2019-10-16T17:38:49Z

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

Trott · 2019-10-16T17:39:57Z

@Trott is the CI on this broken from the commit message or something else?

Just the commit message. If you can rebase and fix it, great. But if not, whoever lands this can also fix it.

aug2uag · 2019-10-22T05:29:53Z

@Trott update ready

bnoordhuis · 2019-10-22T11:54:27Z

src/node_stat_watcher.h

@@ -26,7 +26,7 @@

 #include "node.h"
 #include "handle_wrap.h"
-#include "env.h"
+#include "env-inl.h"


Sorry, I should have explained it more clearly last time. Compilation units (*.cc files) use env-inl.h but header files should normally use plain env.h because they only need the declarations, not inline definitions. My apologies!

edit: that also means src/node_stat_watcher.cc should keep including env-inl.h.

@bnoordhuis node_stat_watcher.h and node_stat_watcher.cc both import env.h

should this be (1) left as is or (2) only remove env.h from implementation file or (3) change implementation import to env-inl.h while keeping env.h in the header file or other? just a little confusing

Option (3) :-)

Thank you and for your patience, very sorry about all the above.

bnoordhuis

LGTM, thanks! Do you want to update src/module_wrap.cc and src/node_report_module.cc too?

aug2uag · 2019-11-30T20:54:07Z

Anna, sorry I just did the same with the other 29988 (i missed your comment here) .. there were the conflicts that I thought needed to be committed-- i didn't mean to cause you trouble.

aug2uag · 2019-11-30T21:02:58Z

@addaleax i tried rebasing but getting that changes are up to date, dunno what to do

addaleax · 2019-11-30T22:14:52Z

@aug2uag Are you rebasing against nodejs/node’s master branch or your own? If you have the nodejs/node repository set as the upstream remote, then git fetch upstream && git rebase upstream/master should help… otherwise, idk but it might be causing troubles that you opened a PR from your repo’s own master branch?

Also, somebody else can do the rebase for you if you prefer.

aug2uag · 2019-11-30T22:28:40Z

@addaleax that was it thank you, i followed the prompts and ended up pushing again-- i hope the CI and team aren't too upset here

addaleax · 2019-11-30T22:35:43Z

@aug2uag Nobody’s upset, don’t worry about that :) It looks like some other unrelated changes from another PR have slipped in there, though?

aug2uag · 2019-12-01T01:26:24Z

@addaleax thanks, also for your great presentations. it's extraordinary to be interacting with you all on this project. i'm here in case there's anything i can do on my part

nodejs-github-bot · 2019-12-01T01:29:16Z

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

gireeshpunathil · 2019-12-01T07:22:48Z

the failure sequential/test-http-server-keep-alive-timeout-slow-client-headers is known as in #22147

gireeshpunathil · 2019-12-01T07:31:37Z

while trying to land this, I get patch is empty message. Looking at the PR changes and the state of upstream/master, it cannot be true; does anyone who what to do?

#curl -L https://github.com/nodejs/node/pull/29975.patch | git am --whitespace=fix
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   109    0   109    0     0    188      0 --:--:-- --:--:-- --:--:--   188
100 96865    0 96865    0     0  56496      0 --:--:--  0:00:01 --:--:--  444k
Patch is empty.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

aug2uag · 2019-12-01T07:50:21Z

@gireeshpunathil it may be artifacts from my misuse of git, the change may be merged from a clean start if it helps

gireeshpunathil · 2019-12-01T07:53:13Z

@aug2uag - no worries at all; I don't think that is the case, but I even don't know how to solve it. Let us wait till some one comes to resolve it!

PR-URL: #29976 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

addaleax · 2019-12-01T10:47:00Z

Landed in 5473ceb :)

while trying to land this, I get patch is empty message. Looking at the PR changes and the state of upstream/master, it cannot be true; does anyone who what to do?

You pointed to the URL for the issue, not the PR 🙂

Trott · 2019-12-01T11:25:25Z

Landed in 5473ceb :)

The entire commit message is src: rebase for 29976 (which is this PR number). That's confusing, or at least I find it confusing? Too late to force push to improve it? I had suggested this:

src: remove unnecessary header file

src/node_stat_watcher.cc imports `env.h` but it is not necessary to do so.

Fixes: https://github.com/nodejs/node/issues/29975

change src/node_stat_watcher.cc to import `env-inl.h` instead of `env.h`. PR-URL: #29976 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Trott · 2019-12-01T11:37:29Z

It's a holiday weekend in the U.S. (and the middle of the night in the Americas), and the weekend everywhere else (well, except in two time zones where it's before 2AM on Monday morning), and things are slow so I force-pushed this as the commit message:

src: change header file in node_stat_watcher.cc

change src/node_stat_watcher.cc to import `env-inl.h` instead of
`env.h`.

PR-URL: https://github.com/nodejs/node/pull/29976
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

addaleax · 2019-12-01T11:38:35Z

@Trott thanks for noticing and fixing this!

change src/node_stat_watcher.cc to import `env-inl.h` instead of `env.h`. PR-URL: #29976 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Oct 14, 2019

Trott mentioned this pull request Oct 15, 2019

test: fix flaky doctool and test #29979

Merged

2 tasks

aug2uag force-pushed the master branch from 812b8fc to 628a9f2 Compare October 15, 2019 09:03

aug2uag force-pushed the master branch from 628a9f2 to cc360ad Compare October 15, 2019 21:04

aug2uag force-pushed the master branch from cc360ad to 6c7b0da Compare October 16, 2019 23:36

bnoordhuis reviewed Oct 22, 2019

View reviewed changes

aug2uag force-pushed the master branch from 6c7b0da to 28168ef Compare October 24, 2019 21:26

bnoordhuis approved these changes Oct 26, 2019

View reviewed changes

src: rebase for 29976

d2862af

aug2uag force-pushed the master branch from 4b189a0 to d2862af Compare December 1, 2019 01:23

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 1, 2019

gireeshpunathil approved these changes Dec 1, 2019

View reviewed changes

addaleax closed this Dec 1, 2019

BridgeAR mentioned this pull request Dec 3, 2019

v13.3.0 proposal #30774

Merged

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

Fixes: https://github.com/nodejs/node/issues/29975 #29976

Fixes: https://github.com/nodejs/node/issues/29975 #29976

Conversation

aug2uag commented Oct 14, 2019

Checklist

Trott commented Oct 14, 2019

Trott commented Oct 14, 2019

joyeecheung commented Oct 14, 2019

aug2uag commented Oct 14, 2019

bnoordhuis commented Oct 14, 2019

nodejs-github-bot commented Oct 15, 2019

nodejs-github-bot commented Oct 15, 2019

nodejs-github-bot commented Oct 15, 2019

nodejs-github-bot commented Oct 15, 2019

nodejs-github-bot commented Oct 15, 2019

nodejs-github-bot commented Oct 15, 2019

nodejs-github-bot commented Oct 15, 2019

Trott commented Oct 15, 2019

aug2uag commented Oct 15, 2019

aug2uag commented Oct 15, 2019

Trott commented Oct 15, 2019

aug2uag commented Oct 15, 2019

aug2uag commented Oct 16, 2019

nodejs-github-bot commented Oct 16, 2019

Trott commented Oct 16, 2019

aug2uag commented Oct 22, 2019

bnoordhuis Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

aug2uag Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

bnoordhuis Oct 24, 2019

Choose a reason for hiding this comment

aug2uag Oct 24, 2019

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

aug2uag commented Nov 30, 2019

aug2uag commented Nov 30, 2019

addaleax commented Nov 30, 2019

aug2uag commented Nov 30, 2019

addaleax commented Nov 30, 2019

aug2uag commented Dec 1, 2019

nodejs-github-bot commented Dec 1, 2019

gireeshpunathil commented Dec 1, 2019

gireeshpunathil commented Dec 1, 2019

aug2uag commented Dec 1, 2019

gireeshpunathil commented Dec 1, 2019

addaleax commented Dec 1, 2019

Trott commented Dec 1, 2019 • edited Loading

Trott commented Dec 1, 2019 • edited Loading

addaleax commented Dec 1, 2019

bnoordhuis Oct 22, 2019 •

edited

Loading

aug2uag Oct 24, 2019 •

edited

Loading

Trott commented Dec 1, 2019 •

edited

Loading

Trott commented Dec 1, 2019 •

edited

Loading