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

[WIP][FileSystem][Enhancement] Fallback to mklink for symlinks/junctions #18324

Closed
wants to merge 1 commit into from

Conversation

c33s
Copy link

@c33s c33s commented Mar 27, 2016

Q A
Branch? 2.3
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets -
License MIT
Doc PR -

currently creating symlinks with php on windows are a real pain. a console with admin rights is needed
or you have to change the privileges for "Create symbolic links" (more information on that, and some more info to that)

i use "symlinks" (junctions) since 2011, with the great tool junction.exe from sysinternals. this tool simply work.
it can only handle directories and no files, but most of the time this is exactly what you need.

running a console as administrator or changing the group policy privileges is not always possible but putting a simple exe file in a directory which is in a directory refered in th environmental path variable, is often possible and easy.

so i made a small POC change in the filesystem component to allow fallback to junction on windows to ask if such a pull request would be accepted or if it have to live as hack (not that easy to replace the filesystem component in a symfony project)

currently i "overload" the filesystem class with composer:

autoload:
    classmap:
        - src/C33s/FileSystem.php
        - app/AppKernel.php
    psr-4:
        '': src/

things todo:

  • only try to run on windows
  • only run if symlink() failed
  • error handling
  • check if source is a directory and not a file
  • maybe introduce a third parameter to symlink "fallbackToJunctionOnWindows"
  • tests
  • check if relative links are possible
  • if relative links are possibble adapt assets:install to not force copy on windows
  • ability to check if is_link()

@nicolas-grekas
Copy link
Member

Shouldn't we use mklink instead of junction? mklink is shipped by default since Vista, whereas junction.exe need a manual installation. (see e.g. http://superuser.com/questions/752538/mklink-vs-junction-exe)

@c33s
Copy link
Author

c33s commented Mar 27, 2016

@nicolas-grekas i am also fine with mklink but then we have to make sure to run it in junction mode to not have the same problems as the php symlink() function.

λ mklink /j /d .\web\example .\source
Junction created for .\web\example <<===>> .\source

vs

λ mklink /d .\web\example .\source
You do not have sufficient privilege to perform this operation.

@c33s
Copy link
Author

c33s commented Mar 27, 2016

one drawback of using mklink is that it is build into cmd.exe, so no possibility to detect it with ExecutableFinder
http://superuser.com/questions/98420/mklink-not-installed-on-windows-7

@nicolas-grekas
Copy link
Member

Given that mklink is available since Vista, we could IMHO safely state that this feature requires it without excluding anyone. Not a blocker for me.

@nicolas-grekas
Copy link
Member

Note that this would be a new feature, to be added to master, not 2.3

@c33s c33s changed the title [POC][FileSystem][Enhancement] Fallback to junction.exe for symlink [WIP][FileSystem][Enhancement] Fallback to mklink for symlinks/junctions Mar 31, 2016
@c33s
Copy link
Author

c33s commented Mar 31, 2016

@nicolas-grekas a few questions:

Overview

  1. Intro
  2. Accept PR?
  3. Need more conceptual guidelines/Support
  4. islink Definition
  5. current state

ad 1) intro

reading your note was quite a setback for me. personally i think the option to create reliable symlinks/junkctions on windows is a bug and not a new feature.

hardcopy is really no option. if you have a complete data repo (~5gb) you want to symlink into your var directory the fallback hardlink option is no option. so its everytime a manual creation of symlinks.

also it is a problem that this bugfix cannot easily/clean manually be backported to the a symfony version by a user. as the filesystem component is not installed as seperate component but with "symfony/symfony" which replaces "symfony/filesytem" the usual composer way of using a custom fork does not work.

see also:

...
repositories:
  -
    type: vcs
    url: 'https://github.com/c33s/filesystem'
...
require:
    php: '>=5.3.9'
    symfony/symfony: '2.8.*'
    doctrine/orm: ^2.4.8
    doctrine/doctrine-bundle: ~1.4
    symfony/swiftmailer-bundle: ~2.3
    symfony/monolog-bundle: ~2.4
    sensio/distribution-bundle: ~5.0
    sensio/framework-extra-bundle: ^3.0.2
    incenteev/composer-parameter-handler: ~2.0
    symfony/filesystem: dev-c33s-fs-win-symlink
...

this leads to a Your requirements could not be resolved to an installable set of packages. error with composer.

...
autoload:
    classmap:
        - src/C33s/Filesystem.php #overwrites the filessytem component to allow windows symlinks
        - app/AppKernel.php 
...

so my current "solution" is quite uncool and leads to a composer warning.

Warning: Ambiguous class resolution, "Symfony\Component\Filesystem\Filesystem" was found in both "vendor/symfony/symfony/src/Symfony/Component/Filesystem/Filesystem.php" and "C33s/Filesystem.php", the first will be used.

please lets call this a bug and let more symfony versions benefit of it. is there a room for discussion to get it in 2.3 or at least into 2.8? maybe add a feature flag for this, to allow user to enable or disable the bugfix/new feature.

ad 2) Accept PR?

as i noticed that this PR is quite a lot of work, especially creating tests and work around the is_link() issues, my main question is, if this PR can find its way in symfony. as you wrote that it will handled as new feature and will be added to master i assume yes but i just want to be sure before invest too much time :) (of course the quality criterias have to be met)

ad 3) Need more conceptual guidelines/Support

as i played around with mklink and is_link i noticed that there are a lot of possibilities how to solve the problems.

  • how to handle mklink detection? should we try to detect it by making a dummy call and after that make the real call? should we call it with cmd.exe to workaround missing mklink in powershell?
  • should we use caching for detected binaries or should the filesystem component stay as simple and slick as is?
  • should we add OS detection?
  • is directory symlink enough or is file symlink also important (for the first step)?
  • is it ok to add helper methods or should everything stay in one method (i am quite a fan of more methods)?
  • should we discuss this PR in a chat?
  • how to test? the tests require windows else the component will simply fallback to the current behavior.
  • how to create test data (also for isLink)? we need to automatically create links and broken links. which can't be easily done without the create link permission on the build server. any ideas?
  • testing on different versions of windows? i currently run win7x64.

ad 4) isLink() Definition

the current is_link() function of php is not reliable on windows. i made a workaround using some tricks i found on the net. we need to check correctly for a link, that symfony can decide if a link creation was successful or not. so i will also need to replace is_link() usage in this component and add a own implementation of it.

the following table shows the output of different functions and methods available.

should be link is my current definition what should be a link and what not. how to handle a broken hardlink? is it a link or a file? isLink is the result of my custom isLink function i added. it already works (so the NULL values are not correct here, the column now shows the same result as should be link. filetype is the php function, also is_dir, is_link and readlink. lstat and stat are also the php functions but only showing the result which is different. array_diff is the diff of the returned arrays of lstat and stat. file_exists is again the php function.

i currently implemented the isLink check like this: https://gist.github.com/c33s/2f238ed200b4ea2e3d3da87c512f2597#file-islink-php (i am sure it can be optimized but currently it works)

+--------------------------+----------------+--------+----------+--------+---------+----------------------------------+--------------------+--------------------+--------------------+-------------+
| filename                 | should be link | isLink | filetype | is_dir | is_link | readlink                         | lstat              | stat               | array_diff         | file_exists |
+--------------------------+----------------+--------+----------+--------+---------+----------------------------------+--------------------+--------------------+--------------------+-------------+
| broken-hardlink-file.txt | false          | NULL   | file     |        | false   | path: 'broken-hardlink-file.txt' |   'mode' => 33206, |   'mode' => 33206, | empty array        | true        |
|                          |                |        |          |        |         |                                  |   'size' => 0,     |   'size' => 0,     |                    |             |
| broken-symlink-file.txt  | false          | NULL   | link     |        | true    | false                            |   'mode' => 41398, |   'mode' => 33206, |   'mode' => 33206, | false       |
|                          |                |        |          |        |         |                                  |   'size' => 0,     |   'size' => 0,     |                    |             |
| dir_broken_junction      | false          | NULL   | unknown  | 1      | false   | false                            |   'mode' => 0,     |   'mode' => 16895, |   'mode' => 16895, | false       |
|                          |                |        |          |        |         |                                  |   'size' => 0,     |   'size' => 0,     |                    |             |
| dir_junction             | true           | NULL   | unknown  | 1      | false   | path: 'junction'                 |   'mode' => 0,     |   'mode' => 16895, |   'mode' => 16895, | true        |
|                          |                |        |          |        |         |                                  |   'size' => 0,     |   'size' => 0,     |                    |             |
| dir_realdir              | false          | NULL   | dir      | 1      | false   | path: 'dir_realdir'              |   'mode' => 16895, |   'mode' => 16895, | empty array        | true        |
|                          |                |        |          |        |         |                                  |   'size' => 0,     |   'size' => 0,     |                    |             |
| dir_symlink              | true           | NULL   | link     | 1      | true    | path: 'symlink'                  |   'mode' => 41398, |   'mode' => 16895, |   'mode' => 16895, | true        |
|                          |                |        |          |        |         |                                  |   'size' => 0,     |   'size' => 0,     |                    |             |
| hardlink-file.txt        | true           | NULL   | file     |        | false   | path: 'hardlink-file.txt'        |   'mode' => 33206, |   'mode' => 33206, | empty array        | true        |
|                          |                |        |          |        |         |                                  |   'size' => 2,     |   'size' => 2,     |                    |             |
| realfile.txt             | false          | NULL   | file     |        | false   | path: 'realfile.txt'             |   'mode' => 33206, |   'mode' => 33206, | empty array        | true        |
|                          |                |        |          |        |         |                                  |   'size' => 0,     |   'size' => 0,     |                    |             |
| symlink-file.txt         | true           | NULL   | link     |        | true    | path: 'symlink-file.txt'         |   'mode' => 41398, |   'mode' => 33206, |   'mode' => 33206, | true        |
|                          |                |        |          |        |         |                                  |   'size' => 0,     |   'size' => 0,     |                    |             |
+--------------------------+----------------+--------+----------+--------+---------+----------------------------------+--------------------+--------------------+--------------------+-------------+

same table as ghist: https://gist.github.com/c33s/2f238ed200b4ea2e3d3da87c512f2597#file-is-link-check-txt

ad 5) Current state

the current state of this PR is work in progress. i play around on a local project, which uses windows and i deploy to a linux server over a linux CI server.
i have to verify that mklink does not delete any source files. had this while using relative links but i think it was the work in progress state before.

current code (i develop it in a real project and as i stated above the overriding via composer is not possible. shouldn't symfony allow better overriding of components?):

@nicolas-grekas
Copy link
Member

@c33s that's a great analysis thanks!

For your first point, I suggest we don't discuss if it's a bug or a feature here but wait for this discussion to decide (and contribute to it meanwhile). What is sure is that if you add any new public or protected methods to the Filesystem class, it will qualify as a feature. So that's our main boundary: no change in any public interface.
And what is sure also is that if you can do it, we want this into Symfony! :-)

how to handle mklink detection? [...] should we call it with cmd.exe?
should we add OS detection?

there is no need to bother: let's call it and if it fails, fallback to the current way. PHP always executes processes through cmd.exe so the command will almost always be here.

should we use caching for detected binaries

maybe later but I don't think it will provide much benefit (since I expect mklink to almost always work).

is directory symlink enough or is file symlink also important (for the first step)?

directory symlink is the only feature provided by Filesystem->symlink, we're good!

is it ok to add helper methods or should everything stay in one method

these would be new features, thus on master only

how to test?

we use appveyor testing for every PR, which runs PHP on Windows already

how to create test data (also for isLink)?

this is also already tested and done in the existing test suite, and run on appveyor where the required privilege is set

testing on different versions of windows? i currently run win7x64.

no need, appveyor again

  1. isLink() Definition

you could add a private isLink method on 2.3, but a public one would need to be on master

the current is_link() function of php is not reliable on windows.

if fact, it is used a few times in the component with required workarounds when it returns false but should return true. I don't know if we need a feature full isLink implementation for these cases. Of course, that would be required if the method were made public, but since your target is 2.3, maybe not.

You could add the public method on master later, after 2.3 is made to work here with mklink.

@nicolas-grekas
Copy link
Member

I spent a bit of time playing with junctions+php on Windows.
I can't reproduce the diff between lstat and stat (test script below).
On PHP 5.3.3 (lowest supported version on 2.3), is_link, filetype, etc. behave differently.
The behavior you document starts at 5.3.4.
Your isLink should return true even when the jonction is broken (as on Linux).
Both the symlink and the mirror methods would need a patch to handle jonctions as fallback to symbolic links.
For the mirror method, it means the recursive iterator needs to skip jonctions and also be able to duplicate them so that the mirrored ones link to a dir inside the mirror when applicable.

<?php

@mkdir('tata');
echo exec('mklink /J toto tata'), "\n";
echo 'realpath: '.realpath('./toto'), "\n";
echo 'file_exists: '.file_exists('./toto'), "\n";
echo 'is_link: '.@is_link('./toto'), "\n";
echo 'readlink: '.@readlink('./toto'), "\n";
echo 'filetype: '.@filetype('./toto'), "\n";
print_r(array_diff(lstat('./toto'), stat('./tata')));
echo 'unlink: '.@unlink('./toto'), "\n";
echo 'rmdir: '.@rmdir('./toto'), "\n";

echo exec('mklink /J toto tata'), "\n";
@rmdir('tata');
echo 'realpath: '.realpath('./toto'), "\n";
echo 'file_exists: '.file_exists('./toto'), "\n";
echo 'is_link: '.@is_link('./toto'), "\n";
echo 'readlink: '.@readlink('./toto'), "\n";
echo 'filetype: '.@filetype('./toto'), "\n";
print_r(array_diff(lstat('./toto'), stat('./tata')));
echo 'unlink: '.@unlink('./toto'), "\n";
echo 'rmdir: '.@rmdir('./toto'), "\n";

@fabpot
Copy link
Member

fabpot commented Apr 28, 2016

@c33s Any feedback?

@c33s
Copy link
Author

c33s commented Apr 29, 2016

@fabpot currently to much work to do, should be able to continue working on this in a few weeks

@fabpot
Copy link
Member

fabpot commented Jun 14, 2016

@c33s Sorry to be pushy, but do you think you will be able to finish this one soon? If not, we can create an issue referencing this PR and close it so that anyone interested can take over your work. Thanks.

@fabpot
Copy link
Member

fabpot commented Nov 9, 2016

Closing this PR as there i no activity and it's far from being ready.

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.

None yet

5 participants