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

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

Closed
wants to merge 1 commit into from

Conversation

aug2uag
Copy link
Contributor

@aug2uag aug2uag commented Oct 14, 2019

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

@nodejs-github-bot 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
Copy link
Member

Trott commented Oct 14, 2019

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
Copy link
Member

Trott commented Oct 14, 2019

@joyeecheung

@joyeecheung
Copy link
Member

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
Copy link
Contributor Author

aug2uag commented Oct 14, 2019

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
Copy link
Member

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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Oct 15, 2019

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

@Trott Trott mentioned this pull request Oct 15, 2019
2 tasks
@aug2uag
Copy link
Contributor Author

aug2uag commented Oct 15, 2019

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
Copy link
Contributor Author

aug2uag commented Oct 15, 2019

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
Copy link
Member

Trott commented Oct 15, 2019

@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
Copy link
Contributor Author

aug2uag commented Oct 15, 2019

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
Copy link
Contributor Author

aug2uag commented Oct 16, 2019

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

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Oct 16, 2019

@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
Copy link
Contributor Author

aug2uag commented Oct 22, 2019

@Trott update ready

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

#include "node.h"
#include "handle_wrap.h"
#include "env.h"
#include "env-inl.h"
Copy link
Member

@bnoordhuis bnoordhuis Oct 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@aug2uag aug2uag Oct 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option (3) :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aug2uag
Copy link
Contributor Author

aug2uag commented Nov 30, 2019

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
Copy link
Contributor Author

aug2uag commented Nov 30, 2019

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

@addaleax
Copy link
Member

@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
Copy link
Contributor Author

aug2uag commented Nov 30, 2019

@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
Copy link
Member

@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
Copy link
Contributor Author

aug2uag commented Dec 1, 2019

@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

@addaleax 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
@nodejs-github-bot
Copy link
Collaborator

@gireeshpunathil
Copy link
Member

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

@gireeshpunathil
Copy link
Member

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
Copy link
Contributor Author

aug2uag commented Dec 1, 2019

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

@gireeshpunathil
Copy link
Member

@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!

addaleax pushed a commit that referenced this pull request Dec 1, 2019
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
Copy link
Member

addaleax commented Dec 1, 2019

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 🙂

@addaleax addaleax closed this Dec 1, 2019
@Trott
Copy link
Member

Trott commented Dec 1, 2019

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

Trott pushed a commit that referenced this pull request Dec 1, 2019
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
Copy link
Member

Trott commented Dec 1, 2019

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
Copy link
Member

addaleax commented Dec 1, 2019

@Trott thanks for noticing and fixing this!

targos pushed a commit that referenced this pull request Dec 2, 2019
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>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
targos pushed a commit that referenced this pull request Jan 13, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants