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

fs: fix subdirectory detection logic #54288

Closed
wants to merge 2 commits into from

Conversation

avivkeller
Copy link
Member

Fixes #54285

Checks that std::filesystem::path::preferred_separator exists after a directory for detecting subdirectories.

@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. needs-ci PRs that need a full CI run. labels Aug 9, 2024
Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.89%. Comparing base (9e6c526) to head (157004e).
Report is 447 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54288      +/-   ##
==========================================
+ Coverage   87.10%   87.89%   +0.78%     
==========================================
  Files         647      651       +4     
  Lines      181754   183366    +1612     
  Branches    34880    35718     +838     
==========================================
+ Hits       158323   161167    +2844     
+ Misses      16742    15468    -1274     
- Partials     6689     6731      +42     
Files with missing lines Coverage Δ
src/node_file.cc 76.91% <66.66%> (+0.06%) ⬆️

... and 196 files with indirect coverage changes

@avivkeller
Copy link
Member Author

CI failure appears unrelated

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@anonrig anonrig requested review from mcollina and jasnell August 14, 2024 23:51
@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@avivkeller
Copy link
Member Author

This has been author ready for a while, can it land?

@nodejs-github-bot

This comment was marked as outdated.

@ikemo3
Copy link

ikemo3 commented Sep 3, 2024

I'm encountering an issue with the CI job, and I'm not quite sure what it means. Here is the relevant error message:

https://ci.nodejs.org/job/node-test-commit-windows-fanned/65275/console

06:47:00 + git archive --format=tar [email protected]:binary_tmp.git jenkins-node-test-commit-windows-fanned-f777812c7105333bbd212153ddcaaee38b4c1f7c src/node_version.h -o node_version.h.tar
06:47:00 Warning: Permanently added '67.158.54.159' (ED25519) to the list of known hosts.
06:47:00 remote: fatal: no such ref: jenkins-node-test-commit-windows-fanned-f777812c7105333bbd212153ddcaaee38b4c1f7c        
06:47:01 remote: git upload-archive: archiver died with error
06:47:01 fatal: sent error to the client: git upload-archive: archiver died with error

It seems that the git archive command is failing due to a missing reference, but I'm not sure why this is happening or how to resolve it. I don't have the ability to fix CI issues myself, so could someone from the team please take a look?

@avivkeller
Copy link
Member Author

avivkeller commented Sep 4, 2024

Hi! It just appears that the CI failed to start (I think). If someone could restart the windows machine, it'd help this land, thanks!

@nodejs-github-bot

This comment was marked as outdated.

@ikemo3
Copy link

ikemo3 commented Sep 4, 2024

I’m not an internal member, so I can't restart CI. Could you help?
@mcollina @jasnell @anonrig

@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 5, 2024
@nodejs-github-bot

This comment was marked as outdated.

src/node_file.cc Outdated Show resolved Hide resolved
Co-authored-by: Jason Zhang <[email protected]>
@avivkeller avivkeller added the wip Issues and PRs that are still a work in progress. label Sep 11, 2024
@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 18, 2024
@aduh95
Copy link
Contributor

aduh95 commented Sep 18, 2024

RedYetiDev added the wip Issues and PRs that are still a work in progress. label

@redyetidev please remove the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label as soon as the PR is not ready to land.

@avivkeller avivkeller closed this Sep 20, 2024
@avivkeller
Copy link
Member Author

@jazelly If you want to take this up, feel free too.

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++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fs.cpSync fails when copying to a directory with a name starting with the source directory name
8 participants