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.cp/fs.cpSync convert relative symlinks to absolute symlinks #41693

Closed
marcosbc opened this issue Jan 25, 2022 · 3 comments · Fixed by #41819
Closed

fs.cp/fs.cpSync convert relative symlinks to absolute symlinks #41693

marcosbc opened this issue Jan 25, 2022 · 3 comments · Fixed by #41819
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.

Comments

@marcosbc
Copy link
Contributor

marcosbc commented Jan 25, 2022

Version

v16.13.2

Platform

Linux mbdev 4.15.0-142-generic #146~16.04.1-Ubuntu SMP Tue Apr 13 09:27:15 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Create the following folder structure (see example code below):

.
├── test1
│   ├── bar -> foo
│   └── foo
└── test2

This can be achieved with the following commands:

$ mkdir test1 test2
$ touch test1/foo
$ ln -s foo test1/bar

Afterwards, execute the following command in your Node.js console:

$ fs.cpSync('test1', 'test2', {recursive: true});

How often does it reproduce? Is there a required condition?

This happens in all Node.js releases from 16.7.0, where fs.cp and fs.cpSync are supported.

What is the expected behavior?

Given the above example, it would be expected for the test2 folder to contain a relative symlink bar, same than test1, since the action is expected to replicate the entire structure from one folder to another, i.e.:

.
├── test1
│   ├── bar -> foo
│   └── foo
└── test2
    ├── bar -> foo
    └── foo

What do you see instead?

Given the above example, the symlinks are converted to absolute symlinks, and also pointing to the "previous" path, i.e.:

.
├── test1
│   ├── bar -> foo
│   └── foo
└── test2
    ├── bar -> /tmp/nodejs-cp-error/test1/foo
    └── foo

Additional information

No response

@VoltrexKeyva VoltrexKeyva added the fs Issues and PRs related to the fs subsystem / file system. label Jan 25, 2022
marcosbc added a commit to marcosbc/node that referenced this issue Feb 2, 2022
Closes nodejs#41693

Signed-off-by: Marcos Bjoerkelund <marcosbd@vmware.com>
marcosbc added a commit to marcosbc/node that referenced this issue Feb 2, 2022
Closes nodejs#41693

Signed-off-by: Marcos Bjoerkelund <marcosbd@vmware.com>
marcosbc added a commit to marcosbc/node that referenced this issue Feb 2, 2022
Fixes: nodejs#41693

Signed-off-by: Marcos Bjoerkelund <marcosbd@vmware.com>
marcosbc added a commit to marcosbc/node that referenced this issue Feb 2, 2022
@tniessen
Copy link
Member

tniessen commented Feb 2, 2022

.
├── foo
|   ├── target
│   ├── copy_me
│       ├── somelink -> ../target
└── bar
    ├── copy_here

To clarify, what should be the result of copying copy_me to copy_here? Should bar/copy_here/somelink point to ../../foo/target? What if foo and bar are on different Windows drives and no relative path exists that could refer to the original link?

@marcosbc
Copy link
Contributor Author

marcosbc commented Feb 2, 2022

@tniessen I agree that in this case it would not make sense. In our case, we were finding issues because we were distributing a folder created as a result of cpSync, but not the "origin" folder there the files were copied from, so some symlinks would point to the "origin" paths that were not available.

I sent a proposal in #41819 to implement this feature, which could be enabled by passing absolute: false verbatimSymlinks: true to fs.cp or fs.cpSync. In order not to break existing code, I proposed disabling it by default.

@tniessen tniessen added the feature request Issues that request new features to be added to Node.js. label Feb 2, 2022
@tniessen
Copy link
Member

tniessen commented Feb 2, 2022

I am going to mark this as a feature request, not a bug, then.

marcosbc added a commit to marcosbc/node that referenced this issue Feb 8, 2022
nodejs-github-bot pushed a commit that referenced this issue Feb 15, 2022
Fixes: #41693

PR-URL: #41819
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
Fixes: nodejs#41693

PR-URL: nodejs#41819
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
bengl pushed a commit that referenced this issue Feb 21, 2022
Fixes: #41693

PR-URL: #41819
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
bengl pushed a commit that referenced this issue Feb 22, 2022
Fixes: #41693

PR-URL: #41819
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
Fixes: nodejs#41693

PR-URL: nodejs#41819
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fixes: #41693

PR-URL: #41819
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.
Projects
Development

Successfully merging a pull request may close this issue.

3 participants