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

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

Conversation

RafaelGSS
Copy link
Member

@RafaelGSS RafaelGSS commented Dec 27, 2023

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

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @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. labels Dec 27, 2023
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2023
@github-actions 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
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

@RafaelGSS
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

2 participants