FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src,lib: use node_path for path.resolve by RafaelGSS · Pull Request #51295 · nodejs/node · GitHub

/ node Public

src,lib: use node_path for path.resolve - #51295

Closed
RafaelGSS wants to merge 1 commit into
nodejs:feat/pm-resolvefrom
RafaelGSS:lib/use-native-path-resolve-temp
ConversationCommits (1)ChecksFiles changed
Closed

src,lib: use node_path for path.resolve#51295
RafaelGSS wants to merge 1 commit into
nodejs:feat/pm-resolvefrom
RafaelGSS:lib/use-native-path-resolve-temp

Conversation

RafaelGSS commented Dec 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

I'm opening it to assert all the changes made in #50758 as suggested in (#50758 (comment)).

Please, do not merge this PR

This PR intends to validate if the C++ implementation matches the JS one, so I can proceed with #50758 PR.

Since I haven't created the path.win32 implementation, I had to move some tests to Windows only.

Note: I'll exclude upstream feat/pm-resolve branch soon as my PR lands.

nodejs-github-bot commented Dec 27, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/startup

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. labels Dec 27, 2023
RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2023

nodejs-github-bot commented Dec 27, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Dec 27, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Dec 28, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Dec 28, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Dec 28, 2023

Copy link
Copy Markdown
Collaborator

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

RafaelGSS commented Dec 28, 2023

Copy link
Copy Markdown
Member Author

@tniessen I see most of the failures are related to parallel.test-crypto-keygen-key-objects which seems unrelated to this PR (I guess).

Do you know if these crypto functions use path.resolve somehow? I took a look at the implementation and I couldn't find a correlation other than requiring the module itself. I'm assuming it's just flaky on osx.

RafaelGSS mentioned this pull request Dec 28, 2023
Merged
RafaelGSS closed this Dec 30, 2023
RafaelGSS mentioned this pull request Mar 19, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL