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

fix: reliable deletion of node_modules #12822

Merged
merged 8 commits into from
Jan 25, 2022

Conversation

joheriks
Copy link
Contributor

@joheriks joheriks commented Jan 25, 2022

Do not set permissions nor follow symlinks when
deleting node_modules.

Fixes #12810

Set writable permission before deletion to handle symlinks
properly.
@Artur-
Copy link
Member

Artur- commented Jan 25, 2022

This looks like a reversed approach i.e. fixing the issue after it has happened (files created without writable permission). What is causing the files to be created with the wrong permissions?

@joheriks
Copy link
Contributor Author

joheriks commented Jan 25, 2022

This looks like a reversed approach i.e. fixing the issue after it has happened (files created without writable permission). What is causing the files to be created with the wrong permissions?

Actually, setting write permission in the initial pass is an additional measure. It works without it also in my tests, but I added it as I am not sure if pnpm / npm could ever create a file with no user write permission in node_modules. What caused the permissions to get set wrong was when the FileUtils-based deletion failed trying override the permissions of a symlink that pointed to a file that had been deleted.

@github-actions
Copy link

github-actions bot commented Jan 25, 2022

Unit Test Results

   774 files  +3     774 suites  +3   29m 44s ⏱️ + 5m 28s
5 757 tests +7  5 707 ✔️ +9  50 💤  - 1  0 ±0 
5 785 runs  +1  5 734 ✔️ +3  51 💤  - 1  0 ±0 

Results for commit 6fae721. ± Comparison against base commit 7ce3903.

♻️ This comment has been updated with latest results.

Artur-
Artur- previously approved these changes Jan 25, 2022
Copy link
Member

@Artur- Artur- left a comment

Choose a reason for hiding this comment

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

Can this be unit tested?

@joheriks
Copy link
Contributor Author

I will wing together a unit test that deletes a directory containing some internal symbolic links, though it may be hard to get it to actually fail with the previous impl as the file visiting order is non-deterministic.

Artur-
Artur- previously approved these changes Jan 25, 2022
Artur-
Artur- previously approved these changes Jan 25, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@joheriks joheriks enabled auto-merge (squash) January 25, 2022 14:49
@joheriks joheriks merged commit eab25b3 into master Jan 25, 2022
@joheriks joheriks deleted the joheriks/fix-delete-nodemodules-with-symlinks branch January 25, 2022 16:09
vaadin-bot pushed a commit that referenced this pull request Jan 25, 2022
Do not set permissions nor follow symlinks when
deleting node_modules.

Fixes #12810
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 23.0.0.alpha4 and is also targeting the upcoming stable 23.0.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PiT v23: Upgrading from v22, unable to delete file in node_module
4 participants