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

The Filesystem component cannot rename between different drives #12533

Closed
stof opened this issue Nov 21, 2014 · 23 comments
Closed

The Filesystem component cannot rename between different drives #12533

stof opened this issue Nov 21, 2014 · 23 comments

Comments

@stof
Copy link
Member

stof commented Nov 21, 2014

Because it only tries the PHP rename function, the Filesystem component inherits its limitation about being restricted to a single drive.
See https://github.com/symfony/symfony-installer/issues/46#issuecomment-63951753 for an issue caused by that.

composer implemented a fallback logic to be able to rename between drives: https://github.com/composer/composer/blob/edd4b2f984821fa696f1be848f9d5b5aea449537/src/Composer/Util/Filesystem.php#L260
It might be worth integrating it in the Filesystem component

@raulfraile
Copy link
Contributor

Actually, this should be considered as a bug, as it is not the expected behaviour (even though PHP's rename function does not work fine too). I think it is really important to implement what Composer already did, especially for the Symfony installer, as it is quite common to have different partitions in Windows, one for the system (with PHP installed, where temp files are created) and other for stuff such as web projects.

@pborreli
Copy link
Contributor

Il I understand correctly, it's a Windows specific issue
I will spend SymfonyCon Madrid hacking day to try to fix some Windows related issues, it would be great if we could have a specific label for all those bugs.

@stof
Copy link
Member Author

stof commented Nov 21, 2014

@pborreli no. cross-drives renaming are not possible on Linux either AFAIK. See the Composer code. It has the fallback for both Windows and Unix. See composer/composer#900 for the Unix report in Composer

@stof
Copy link
Member Author

stof commented Nov 21, 2014

And fixing this is probably just a matter of reusing the Composer logic (licenses are compatible, and Composer is using this code since 2 years)

@pborreli
Copy link
Contributor

ok @stof, thanks.

@RoSk0
Copy link
Contributor

RoSk0 commented Nov 24, 2014

Can confirm that using rename() PHP function here is the root of the issue for Linux with separated / and /home drives.

@Pierstoval
Copy link
Contributor

@RoSk0 👍 , tested on three debian distribs this morning, got an error. But using copy + unlink works well.

@xabbuh
Copy link
Member

xabbuh commented Nov 26, 2014

@Pierstoval I wasn't able to reproduce this on Ubuntu. Is there something special I have to do to get the error?

By the way, the PHP documentation states that moving across device boundaries should be possible:

5.3.1 rename() can now rename files across drives in Windows.
[...]
4.3.3 rename() may now be able to rename files across partitions on *nix based systems, provided the appropriate permissions are held.

@RoSk0
Copy link
Contributor

RoSk0 commented Nov 26, 2014

@xabbuh Looks like the issue here is in internal rename() implementation.
Trying to create new project in Ubuntu system configure to have separated / and /home drives leading to this:

Warning: rename(): The first argument to copy() function cannot be a directory in /home/rosko/projects/symfony-installer/vendor/symfony/filesystem/Symfony/Component/Filesystem/Filesystem.php on line 263

rename() works good with files, but when it comes to directories...
We should figure out another approach for directories and the comment #12533 (comment) looks like a good solution.

RoSk0 added a commit to RoSk0/symfony that referenced this issue Nov 26, 2014
RoSk0 added a commit to RoSk0/symfony that referenced this issue Nov 26, 2014
@desarrolla2
Copy link
Contributor

@stof but the proposal composer solution uses the component process should include the PR component dependence proces or simply run operating system calls via exec, I could work on this.

@Pierstoval
Copy link
Contributor

Do you think it may be a good idea to make multiple tries in copying and if none works, throw the exception ? With this, we may be able to log a warning when first try does not work and the next one does.

For example, first try may be basically using rename() function , if it does not work, we send a not-blocking warning in either possible way (logging if available, echoing if verbosity is set, etc.) , and the second try may be a simple copy() + unlink(), and maybe find other solutions ?
It looks heavy, of course, but it may be a solution to solve the problem with different drives or with renaming directories ... ❔

@xabbuh
Copy link
Member

xabbuh commented Dec 1, 2014

@Pierstoval This is basically what Composer does (see the code @stof linked to in the issue description).

@xabbuh
Copy link
Member

xabbuh commented Dec 2, 2014

@Pierstoval Are you working on this?

@Pierstoval
Copy link
Contributor

Nope, I have too much work to do on my different apps to work on this issue :(

@desarrolla2
Copy link
Contributor

I would like to send a PR, but should i use the process component for system calls, or call exec() funcion?

@xabbuh
Copy link
Member

xabbuh commented Dec 3, 2014

@desarrolla2 From my point of view, adding a dependency on the Process component only for this edge case doesn't make much sense. Checking first if the ProcessBuilder class is available before falling back to a low-level functions seems to be too much overhead to me.

@desarrolla2
Copy link
Contributor

@xabbuh ok, i will work on this and send a PR.

@raulfraile
Copy link
Contributor

@xabbuh @desarrolla2 some components like Finder or Intl are already using exec() for some operations, so I also think that depending on the Filesystem component (hard or soft) does not make much sense and is not consistent.

weaverryan added a commit to weaverryan/distill that referenced this issue Dec 7, 2014
@xabbuh
Copy link
Member

xabbuh commented Dec 29, 2014

@desarrolla2 Are you still working on this?

@desarrolla2
Copy link
Contributor

ups, i forget the issue, i will work on this this night.

@raulfraile
Copy link
Contributor

@desarrolla2 let me know if you can't work on it soon so I can take over the issue

@LewisW
Copy link

LewisW commented Feb 27, 2015

Just for reference if anyone is using Docker, there's a high chance they will come across this problem if their cache directory is originally created in a separate layer (i.e. at the build stage).

@andrerom
Copy link
Contributor

andrerom commented Jun 4, 2016

Can confirm this issue during docker build, had to do a rm -Rf app/logs/* app/cache/* first in the RUN step that triggered this indirectly via composer install.

@fabpot fabpot closed this as completed Jun 14, 2017
klammbueddel pushed a commit to klammbueddel/common that referenced this issue Jul 1, 2019
...to move a file without side effects.

This gist reproduces the problem:
https://gist.github.com/alcaeus/a367e895e6c55f7fb93870dcba46efa9

Same issue is fixed in a similary way in symfony core:
symfony/symfony#12533

resolves #6713
klammbueddel pushed a commit to klammbueddel/common that referenced this issue Jul 1, 2019
...to move a file without side effects.

This gist reproduces the problem:
https://gist.github.com/alcaeus/a367e895e6c55f7fb93870dcba46efa9

Same issue is fixed in a similary way in symfony core:
symfony/symfony#12533

resolves doctrine/orm#6713
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants