-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
lib,src,permission: port path.resolve to C++ #50758
Conversation
Review requested:
|
Why doesn't it remove the JS implementation. I feel it's very dangerous to have two impl. that are used in different places |
That's the idea, but I'd prefer to do it in a follow-up PR, so we can measure performance, impacts and it will make the review easier. |
c7ac444
to
b2649a3
Compare
Do we know if this implementation is actually equivalent to the JS implementation? Do any of the tests fail if we replace the JS one with this? I think it should be fairly easy to verify if you just add a binding (maybe in |
It must be. The tests are actually equivalents. |
Did you mean the unit tests? I am under the impression that for basic APIs those are mostly smoking tests, but path.resolve is used internally quite a lot so the correctness is largely checked via all the indirect usages (e.g. some test might just fail on Windows because the API it uses indirectly uses path.resolve(), or the module loader uses it). It would be better to verify the correctness of the alternative using this form of coverage. |
@Ceres6 will be looking to the windows errors. I can create the binding test suggested by Joyee. |
7c36ed1
to
dfac7a2
Compare
I will wait a couple of days before merging this PR, just to guarantee the errors shown in #51295 aren't related to |
Commit Queue failed- Loading data for nodejs/node/pull/50758 ✔ Done loading data for nodejs/node/pull/50758 ----------------------------------- PR info ------------------------------------ Title lib,src,permission: port path.resolve to C++ (#50758) Author Rafael Gonzaga (@RafaelGSS) Branch RafaelGSS:feat/pm-resolve -> nodejs:main Labels c++, lib / src, notable-change, needs-ci, permission Commits 1 - lib,src,permission: port path.resolve to C++ Committers 1 - RafaelGSS PR-URL: https://github.com/nodejs/node/pull/50758 Refs: https://github.com/nodejs/security-wg/issues/898 Reviewed-By: James M Snell Reviewed-By: Paolo Insogna ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/50758 Refs: https://github.com/nodejs/security-wg/issues/898 Reviewed-By: James M Snell Reviewed-By: Paolo Insogna -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - lib,src,permission: port path.resolve to C++ ℹ This PR was created on Thu, 16 Nov 2023 17:12:16 GMT ✔ Approvals: 2 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/50758#pullrequestreview-1773719337 ✔ - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/50758#pullrequestreview-1793165763 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-12-27T18:11:23Z: https://ci.nodejs.org/job/node-test-pull-request/56560/ - Querying data for job/node-test-pull-request/56560/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/7364282650 |
Landed in 338a5be |
Co-Authored-By: Carlos Espa <cespatorres@gmail.com> PR-URL: #50758 Refs: nodejs/security-wg#898 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: TODO
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) nodejs#50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) nodejs#50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) nodejs#51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) nodejs#50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) nodejs#51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) nodejs#51246 PR-URL: nodejs#51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) #50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) #50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) #51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) #50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) #51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) #51246 PR-URL: #51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Notable changes: doc: * (SEMVER-MINOR) add documentation for --build-snapshot-config (Anna Henningsen) nodejs#50453 lib,src,permission: * (SEMVER-MINOR) port path.resolve to C++ (Rafael Gonzaga) nodejs#50758 net: * (SEMVER-MINOR) add connection attempt events (Paolo Insogna) nodejs#51045 src: * (SEMVER-MINOR) support configurable snapshot (Joyee Cheung) nodejs#50453 src,permission: * (SEMVER-MINOR) add --allow-addon flag (Rafael Gonzaga) nodejs#51183 timers: * (SEMVER-MINOR) export timers.promises (Marco Ippolito) nodejs#51246 PR-URL: nodejs#51342 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Co-Authored-By: Carlos Espa <cespatorres@gmail.com> PR-URL: #50758 Refs: nodejs/security-wg#898 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Co-Authored-By: Carlos Espa <cespatorres@gmail.com> PR-URL: #50758 Refs: nodejs/security-wg#898 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Claudio Wunder <cwunder@gnome.org>
We're porting
path.resolve
to C++ to be used on the permission model and other places.Summary: The permission Model now accepts relative paths through the CLI. Example:
--experimental-permission --allow-fs-read=./index.js
Refs: nodejs/security-wg#898