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

[v16.x backport] src: pass only Isolate* and env_vars to EnabledDebugList::Parse() #44070

Conversation

RaisinTen
Copy link
Contributor

Backport of #43668.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v16.x labels Jul 31, 2022
@nodejs-github-bot

This comment was marked as outdated.

@targos
Copy link
Member

targos commented Jul 31, 2022

Thanks. Don't worry too much if there are CI errors. I pushed a new batch of commits today and they were only tested on my mac

LiviaMedeiros and others added 22 commits July 31, 2022 18:10
This allows to support timestamps before 1970-01-01.
On Windows, it's not supported due to Y2038 issue.

PR-URL: nodejs#43714
Fixes: nodejs#43707
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#43881
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs#43872
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Offer additional meta-data for building
custom and additional behaviour on
top of parseArgs.

PR-URL: nodejs#43459
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#43886
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
This issue has been described in -
nodejs#43860

On Windows system, git clone or git checkout on the repo turns LF line
endings to CRLF in the worktree.
This can happen due to many reasons like -
- git config --system core.autocrlf (set to true)
- git config --global core.autocrlf (set to true)
- git config --local core.autocrlf (set to true)
- git clone --config core.autocrlf=true ...

Adding gitattributes for the shims will not convert them to CRLF line
endings.

Also, there is a note[1] in test/README.md which says -
For the tests to run on Windows, be sure to clone Node.js source code
with the `autocrlf` git config flag set to true.

Reason for using build subsystem -
These shims are just copied in stage_package label of vcbuild.bat

Fixes: nodejs#43860
[1]: nodejs@3654cd4

PR-URL: nodejs#43879
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#43870
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
The 'test-tls-env-extra-ca-file-load.js' test assumes that
NODE_EXTRA_CA_CERTS is not set. If the build environment
happens to have it set, this test will fail. This change
deletes that env var before running the test.

PR-URL: nodejs#43858
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Co-authored-by: Rafael Gonzaga <rafael.nunu@hotmail.com>

PR-URL: nodejs#43835
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#43853
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
The shared libraries will now be stores in lib.target as opposed to
obj.target, libnode.version.so, libnode.x (for npm backwards compat and
testing), and libnode.version.x (for builds). The install will also
include libnode.so link that points to libnode.version.so (this will be
used by native npms for backwards compat).

PR-URL: nodejs#42256
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Co-authored-by: Gaby Baghdadi <baghdadi@ca.ibm.com>
Co-authored-by: Wayne Zhang <shuowang.zhang@ibm.com>
This change adds a new parameter `--nvd-key` to `dep_checker`,
which allows the user to specify a NVD API key with which to query
the National Vulnerability Database.

This increases the rate at which we are allowed to query the
database, which speeds up the running time of the script.

PR-URL: nodejs#43909
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#43822
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs#43806
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Refs: nodejs#43897 (comment)

PR-URL: nodejs#43913
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Fixes: nodejs#43839

PR-URL: nodejs#43953
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
the returns need to be lowercase

PR-URL: nodejs#43933
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
PR-URL: nodejs#43927
Fixes: nodejs#43864
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
PR-URL: nodejs#43857
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#43912
Refs: nodejs#39735
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#43922
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#43747
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
codebytere and others added 8 commits July 31, 2022 18:10
PR-URL: nodejs#43779
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Fixes: nodejs#43009

If calling `this._handle.getpeername` returns an error at the first
call, its result shouldn't be cached to `this._peername`.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: nodejs#43010
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Per the comments in nodejs#43924, almost everyone uses `git-node-v8`. I
included example steps for using `git-node-v8`.

I ran through both of these instructions on a clean Linux machine (I had
to fudge the patch SHA of course) and they seemed to work.

Refs: nodejs#43924

PR-URL: nodejs#43934
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Refs: nodejs#43929 (comment)

PR-URL: nodejs#43954
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
In the main thread, `inspector.close` is defined as `process._debugEnd`:

```
> inspector.close
[Function: _debugEnd]
```

It's not defined in worker threads:
```
> const {Worker} = require("worker_threads");
> new Worker("console.log(require(\"inspector\").close)", {eval:true})
undefined
```

(As far as I can tell this is intentional and has existed for quite some
time.)

PR-URL: nodejs#43867
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
PR-URL: nodejs#43958
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#43985
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The function doesn't require access to the entire Environment object, so
this change just passes what it needs.

Addresses this TODO -
https://github.com/nodejs/node/blob/71691e53d601a4cf3eab3b33cebe4cc9f50c8470/src/env.cc#L372-L374

Signed-off-by: Darshan Sen <raisinten@gmail.com>

PR-URL: nodejs#43668
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos targos force-pushed the backport-src-pass-isolate-and-env_vars-to-16 branch from 6cd05b0 to 1073bee Compare July 31, 2022 16:13
@targos
Copy link
Member

targos commented Jul 31, 2022

I removed the problematic commit from staging (19dfa47) and rebased your PR.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Aug 1, 2022

test-fs-stat-date fails consistently on arm-12+. I'll remove the commit from #43714 while landing this.

@targos targos self-assigned this Aug 1, 2022
@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 1, 2022
targos pushed a commit that referenced this pull request Aug 1, 2022
The function doesn't require access to the entire Environment object, so
this change just passes what it needs.

Addresses this TODO -
https://github.com/nodejs/node/blob/71691e53d601a4cf3eab3b33cebe4cc9f50c8470/src/env.cc#L372-L374

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #43668
Backport-PR-URL: #44070
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos
Copy link
Member

targos commented Aug 1, 2022

Landed in 0dc96e4

@targos targos closed this Aug 1, 2022
@RaisinTen RaisinTen deleted the backport-src-pass-isolate-and-env_vars-to-16 branch August 2, 2022 05:31
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
The function doesn't require access to the entire Environment object, so
this change just passes what it needs.

Addresses this TODO -
https://github.com/nodejs/node/blob/71691e53d601a4cf3eab3b33cebe4cc9f50c8470/src/env.cc#L372-L374

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#43668
Backport-PR-URL: nodejs/node#44070
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.