Make assets:install smarter with symlinks #11312

rvanginneken · 2014-07-05T11:21:30Z

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

weaverryan · 2014-07-05T11:56:54Z

src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php

+        $output->writeln('Installing assets.');
+
+        $symLink = function_exists('symlink');
+        if ( !$symLink ) {


Remove the extra spaces here - fabbot.io is reporting this as well :)

weaverryan · 2014-07-05T12:05:04Z

We may need to move this into a new command or only activate this new "figure out the best option" behavior with a new flag (e.g. --auto) depending on if we need to protect BC. I have a feeling that we will need to protect BC, since people have deploy scripts that use this.

weaverryan · 2014-07-05T12:06:06Z

Actually, yes - Fabien just commented about the BC break. Let's try that --auto option and keep symlink on there. Or if someone else has a good suggestion :).

…PHP 5.3 symlink function always exists.

rvanginneken · 2014-07-05T14:14:54Z

Added the --auto option and removed function_exists checks for symlink function.
Also there is an extra check whether an symlinked path actually exists after creation

weaverryan · 2014-07-05T14:50:21Z

src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php

+                        $filesystem->symlink($relativeOriginDir, $targetDir);
+                    } catch (IOException $e) {
+                        if ($input->getOption('auto')) {
+                            $hardCopy = true;


This $hardCopy logic is a little hard to read. It might be easier to read if the actual "hard copy" logic below were moved into a new private function makeHardCopy and then called from here and called from below. But that's minor - if you try it, make sure it does in fact seem clearer to you

Richtermeister · 2014-07-06T20:16:42Z

👍 awesome! I'm on windows and this will be nice.

andrerom · 2014-07-06T22:45:19Z

@Richtermeister Would you be able to test this? With and without the permissions setup to allow to create symlinks on windows (see comments on #11297).

fabpot · 2014-07-07T10:19:47Z

@pborreli Can you try it on your Windows machine?

stof · 2014-07-07T10:23:32Z

src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php

+    /**
+     * Create hardcopy for targetDir
+     */
+    private function hardCopy($originDir, $targetDir, \Traversable $iterator = null)


the iterator seems unused

pborreli · 2014-07-07T10:24:05Z

@fabpot sure I'll test that with multiple env (pure windows, vagrant on windows with and without permissions)

pborreli · 2014-07-07T12:33:31Z

src/Symfony/Bundle/FrameworkBundle/Command/AssetsInstallCommand.php

+                    try {
+                        $filesystem->symlink($relativeOriginDir, $targetDir);
+                    } catch (IOException $e) {
+                        if (!$input->getOption('auto')) {


you mean if ($input->getOption('auto')) {, right ?

yes , seem to have looked over that one

pborreli · 2014-07-07T12:36:26Z

it doesn't work as expected right now :

--auto shows the same exception as before
--symlink works silently (but makes an hardcopy without saying anything)

I added a note in the PR which could explain it.

pborreli · 2014-07-07T13:06:24Z

Works now like expected for Windows machine, give me some time to test it from a vagrant machine on Windows host.

On Windows without any rights

C:\Users\Pascal\Projects\symfony>php app/console assets:install --auto
Trying to install assets as symbolic links.
Installing assets for Symfony\Bundle\FrameworkBundle into web/bundles/framework
Installing assets for Acme\DemoBundle into web/bundles/acmedemo
Installing assets for Sensio\Bundle\DistributionBundle into web/bundles/sensiodistribution
It looks like your system doesn't support symbolic links, so the assets were installed by copying them.

C:\Users\Pascal\Projects\symfony>php app/console assets:install --symlink
Installing assets as symlinks
Installing assets for Symfony\Bundle\FrameworkBundle into web/bundles/framework

  [Symfony\Component\Filesystem\Exception\IOException]
  Unable to create symlink due to error code 1314: 'A required privilege is not held by the client'. Do you have the required Administrator-rights?

assets:install [--symlink] [--auto] [--relative] [target]

C:\Users\Pascal\Projects\symfony>php app/console assets:install
Installing assets as hard copies
Installing assets for Symfony\Bundle\FrameworkBundle into web/bundles/framework
Installing assets for Acme\DemoBundle into web/bundles/acmedemo
Installing assets for Sensio\Bundle\DistributionBundle into web/bundles/sensiodistribution

On Windows machine with admin right :

C:\Users\Pascal\Projects\symfony>php app/console assets:install --auto
Trying to install assets as symbolic links.
Installing assets for Symfony\Bundle\FrameworkBundle into web/bundles/framework
Installing assets for Acme\DemoBundle into web/bundles/acmedemo
Installing assets for Sensio\Bundle\DistributionBundle into web/bundles/sensiodistribution
The assets were installed using symbolic links.

C:\Users\Pascal\Projects\symfony>php app/console assets:install --symlink
Installing assets as symlinks
Installing assets for Symfony\Bundle\FrameworkBundle into web/bundles/framework
Installing assets for Acme\DemoBundle into web/bundles/acmedemo
Installing assets for Sensio\Bundle\DistributionBundle into web/bundles/sensiodistribution

C:\Users\Pascal\Projects\symfony>php app/console assets:install
Installing assets as hard copies
Installing assets for Symfony\Bundle\FrameworkBundle into web/bundles/framework
Installing assets for Acme\DemoBundle into web/bundles/acmedemo
Installing assets for Sensio\Bundle\DistributionBundle into web/bundles/sensiodistribution

andrerom · 2014-07-07T13:20:38Z

Btw, does it make sense to think about combination of --auto and --relative as well here or is only absolute links supported on Windows?

(Advantage og relative symlinks is transportability, however it seems to work poorly with archive formats and maybe also other operations, so might be something to avoid to make it clear they need to be regenerated on move.)

pborreli · 2014-07-07T13:57:16Z

@andrerom you are totally right, if symlink is correctly dealt with, relative option should too.
But ⁉️, right now, relative option doesn't seem to work even if the combination code should:

Tested on Windows:tm: with Admin rights

C:\Users\Pascal\Projects\symfony>php app/console assets:install --auto --relative
Trying to install assets as symbolic links.
Installing assets for Symfony\Bundle\FrameworkBundle into web/bundles/framework
Installing assets for Acme\DemoBundle into web/bundles/acmedemo
Installing assets for Sensio\Bundle\DistributionBundle into web/bundles/sensiodistribution
It looks like your system doesn't support symbolic links, so the assets were installed by copying them.

Removing the try catch block to have the error message:

C:\Users\Pascal\Projects\symfony>php app/console assets:install --auto --relativ
e
Trying to install assets as symbolic links.
Installing assets for Symfony\Bundle\FrameworkBundle into web/bundles/framework

  [Symfony\Component\Filesystem\Exception\IOException]
  Failed to create symbolic link from "../../vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Resources/public/" to "web/bundles/framework".


assets:install [--symlink] [--auto] [--relative] [target]

andrerom · 2014-07-07T14:39:06Z

ok, nice to know.. If it is confirmed/decided to not work, then maybe it should throw early saying the combination is not supported.

pborreli · 2014-07-07T14:42:15Z

@andrerom looks like it's possible http://superuser.com/questions/361826/how-do-you-make-a-symbolic-link-with-a-relative-path-using-mklink indicating the path should be relative to user's working directory instead of related path, worth the try

rvanginneken · 2014-07-07T15:10:19Z

I will give it a try tomorrow, can't get to my src atm. Don't have a windows machine to test BTW.

Pascal Borreli notifications@github.comschreef:

@andreromlookslikeit'spossiblehttp://superuser.com/questions/361826/how-do-you-make-a-symbolic-link-with-a-relative-path-using-mklinkindicatingthepathrelativetouser'sworkingdirectoryinsteadofrelatedpath,worththetryReplytothisemaildirectlyorviewitonGitHub.

Richtermeister · 2014-07-07T16:14:27Z

@andrerom Yes, happy to join the testers.

…ve function mklink instead of symlink

pborreli · 2014-07-13T22:30:22Z

@weaverryan sorry my bad 👊 i edited a file to add a throw and didn't removed it.
works as expected 👍
only missing tests are vagrant

weaverryan · 2014-07-13T22:35:01Z

@pborreli You rock man! Thanks for the fast response!

Ok, I think we just need someone to test with a Vagrant setup. The expected behavior is for the failure of the symlinks to be detected and to fallback to hard-copy with a message.

Anyone have Vagrant setup from a Windows host that can try this?

Thanks!

Swader · 2014-09-07T17:19:56Z

Oh wow, dropped the ball on this, sorry all :(
Trying now.

@weaverryan @andrerom I got through the installation without asset and symlink problems. Looks like we're golden. I'll test further and report my findings, but so far so good. I've got some other DX feedback but I'll shoot that over to Ryan directly.

weaverryan · 2014-09-12T13:07:23Z

@Swader yea, thanks for checking! Was your setup, by chance, a Windows host with Vagrant? That's the last combination that hasn't been tried (but I'm hoping this is what you had).

Thanks for coming back to this - very awesome.

Swader · 2014-09-12T14:07:14Z

Yup, Win 8.1 host, Vagrant guest
On Sep 12, 2014 3:10 PM, "Ryan Weaver" notifications@github.com wrote:

@Swader https://github.com/Swader yea, thanks for checking! Was your
setup, by chance, a Windows host with Vagrant? That's the last combination
that hasn't been tried (but I'm hoping this is what you had).

Thanks for coming back to this - very awesome.

—
Reply to this email directly or view it on GitHub
#11312 (comment).

weaverryan · 2014-09-12T14:12:38Z

Brilliant! We've tested with every reasonable setup that I can think of. I think this PR is ready!

stof · 2014-09-12T15:03:46Z

👍

Tobion · 2014-09-12T15:26:49Z

IMO we can just change the --symlink option to fallback to copying ( = --auto) since it's not considered a bc break. Then we don't need the auto option at all.
Also it would be helpful to display the error message from the exception when the symlink failed.

Tobion · 2014-09-12T15:32:30Z

Reading the comments, I don't see the reasoning behind being a bc break. It would mean a deploy script relied on the command to raise an error when the symlink failed. But when it expects that it fails, it would not have used the --symlink option.

pborreli · 2014-09-12T23:17:12Z

@Swader did you run vagrant as an admin ? or with simple user ?

Swader · 2014-09-13T04:40:42Z

Simple user, non elevated
On Sep 13, 2014 1:17 AM, "Pascal Borreli" notifications@github.com wrote:

@Swader https://github.com/Swader did you run vagrant as an admin ? or
with simple user ?

—
Reply to this email directly or view it on GitHub
#11312 (comment).

pborreli · 2014-09-13T04:59:52Z

@Swader great then 👍

fabpot · 2014-09-16T07:16:16Z

@Tobion idea looks like a good one. If we can avoid creating an option, it's even better.

andrerom · 2014-09-16T08:45:38Z

Maybe rather a --no-auto-symlink option to opt out.

fabpot · 2014-09-16T09:00:57Z

@andrerom I agree with you but as we need to keep BC, @Tobion looks like the best compromise.

andrerom · 2014-09-16T09:33:58Z

ok

weaverryan · 2014-09-16T12:23:46Z

@rvanginneken Can you update the PR so that the nice new behavior happens automatically with the --symlink option (and remove the new auto option)?

Thanks!

rvanginneken · 2014-09-16T12:32:13Z

@weaverryan Yep, I'm still following the thread. I don't have time right now but I'll update it in 1 or 2 days.

weaverryan · 2014-09-16T12:35:16Z

@rvanginneken Perfect, thanks :)

…ality into --symlink

fabpot · 2014-09-22T13:50:00Z

👍

fabpot · 2014-09-22T13:51:49Z

Thank you @rvanginneken.

…neken) This PR was squashed before being merged into the 2.6-dev branch (closes #11312). Discussion ---------- Make assets:install smarter with symlinks | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | - | Fixed tickets | #11297 | License | MIT | Doc PR | - Commits ------- 6537333 Make assets:install smarter with symlinks

…l_extensions smarter with symlinks Mostly same change as in: symfony/symfony#11312 The Symfony change was not backported, so this works best in combination with Symfony 2.6, but will also slightly benefit users on Symfony 2.3 (at least for these commands)

weaverryan reviewed Jul 5, 2014
View reviewed changes

rvanginneken added 2 commits July 5, 2014 15:04

Make assets:install auto

8634199

assets:install:auto removed unnecessary function_exists calls: as of …

f03073e

…PHP 5.3 symlink function always exists.

weaverryan reviewed Jul 5, 2014
View reviewed changes

assets:install cleaned up code

a7330c3

stof reviewed Jul 7, 2014
View reviewed changes

assets:install as mentioned; removed unused itterator from function

2ea12de

pborreli reviewed Jul 7, 2014
View reviewed changes

assets:install removed faulty false check

25b456b

assets:install --auto -> check if running on windows, if so, use nati…

421de05

…ve function mklink instead of symlink

andrerom mentioned this pull request Jul 15, 2014

[DX] Make assets:install smarter with symlinks #11297

Closed

assets:install --auto -> removed --auto param and integrated function…

21c85f9

…ality into --symlink

fabpot closed this Sep 22, 2014

javiereguiluz mentioned this pull request Sep 23, 2014

Symfony 3.0 proposed backward incompatible changes #11742

Closed

derrabus mentioned this pull request Dec 1, 2014

[Filesystem] [BC Break] [2.6] Filesystem component throws exception on broken symlink #12793

Closed

Uh oh!

Make assets:install smarter with symlinks #11312

Make assets:install smarter with symlinks #11312

Uh oh!

Conversation

rvanginneken commented Jul 5, 2014

Uh oh!

weaverryan Jul 5, 2014

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Jul 5, 2014

Uh oh!

weaverryan commented Jul 5, 2014

Uh oh!

rvanginneken commented Jul 5, 2014

Uh oh!

weaverryan Jul 5, 2014

Choose a reason for hiding this comment

Uh oh!

Richtermeister commented Jul 6, 2014

Uh oh!

andrerom commented Jul 6, 2014

Uh oh!

fabpot commented Jul 7, 2014

Uh oh!

stof Jul 7, 2014

Choose a reason for hiding this comment

Uh oh!

pborreli commented Jul 7, 2014

Uh oh!

pborreli Jul 7, 2014

Choose a reason for hiding this comment

Uh oh!

rvanginneken Jul 7, 2014

Choose a reason for hiding this comment

Uh oh!

pborreli commented Jul 7, 2014

Uh oh!

pborreli commented Jul 7, 2014

On Windows without any rights

On Windows machine with admin right :

Uh oh!

andrerom commented Jul 7, 2014

Uh oh!

pborreli commented Jul 7, 2014

Tested on Windows:tm: with Admin rights

Uh oh!

andrerom commented Jul 7, 2014

Uh oh!

pborreli commented Jul 7, 2014

Uh oh!

rvanginneken commented Jul 7, 2014

Uh oh!

Richtermeister commented Jul 7, 2014

Uh oh!

pborreli commented Jul 13, 2014

Uh oh!

weaverryan commented Jul 13, 2014

Uh oh!

Swader commented Sep 7, 2014

Uh oh!

weaverryan commented Sep 12, 2014

Uh oh!

Swader commented Sep 12, 2014

Uh oh!

weaverryan commented Sep 12, 2014

Uh oh!

stof commented Sep 12, 2014

Uh oh!

Tobion commented Sep 12, 2014

Uh oh!

Tobion commented Sep 12, 2014

Uh oh!

pborreli commented Sep 12, 2014

Uh oh!

Swader commented Sep 13, 2014

Uh oh!

pborreli commented Sep 13, 2014

Uh oh!

fabpot commented Sep 16, 2014