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

Make assets:install smarter with symlinks #11312

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
10 participants
@rvanginneken
Copy link
Contributor

commented Jul 5, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? -
Fixed tickets #11297
License MIT
Doc PR -
$output->writeln('Installing assets.');
$symLink = function_exists('symlink');
if ( !$symLink ) {

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 5, 2014

Member

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

$output->writeln(sprintf('Installing assets as <comment>%s</comment>', $input->getOption('symlink') ? 'symlinks' : 'hard copies'));
$output->writeln('Installing assets.');
$symLink = function_exists('symlink');

This comment has been minimized.

Copy link
@fabpot

fabpot Jul 5, 2014

Member

That's wrong. The symlink function always exists, even on Windows. What we need to check is whether we can use it safely or not.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 5, 2014

Member

There is a function_exists call before and one in Filesystem::symlink. I see that as of 5.3.0, symlink looks like it exists for Windows too. Should those checks be removed?

This comment has been minimized.

Copy link
@fabpot

fabpot Jul 5, 2014

Member

yes.

@@ -34,7 +35,6 @@ protected function configure()
->setDefinition(array(
new InputArgument('target', InputArgument::OPTIONAL, 'The target directory', 'web'),
))
->addOption('symlink', null, InputOption::VALUE_NONE, 'Symlinks the assets instead of copying it')

This comment has been minimized.

Copy link
@fabpot

fabpot Jul 5, 2014

Member

That's a serious BC break.

$filesystem->symlink($relativeOriginDir, $targetDir);
} catch (IOException $e) {
$output->writeln(sprintf('Installing assets as symbolic links failed with message <comment>%s</comment>', $e->getMessage()));
}

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 5, 2014

Member

This looks perfect, but I don't think that it'll catch the case where you're on a VM with a Windows host machine, you're sharing the Symfony directory with the host machine, and you don't have proper permissions to actually create symlinks on the Windows machine (see: #11297 (comment)).

The reason I don't think that this will work for that one case is that I don't think these users are currently seeing an IOException - the assets:install just thinks that symlinks work (I could be wrong here, but pretty sure I'm not).

So, I think we need to actually check for the integrity of the symlink - e.g. can you look inside of its contents or not. This could actually go into the Filesystem::symlink method, if we could find a clean way, consistent way of doing this check.

@weaverryan

This comment has been minimized.

Copy link
Member

commented Jul 5, 2014

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

This comment has been minimized.

Copy link
Member

commented Jul 5, 2014

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 :).

rvanginneken added some commits Jul 5, 2014

assets:install:auto removed unnecessary function_exists calls: as of …
…PHP 5.3 symlink function always exists.
@rvanginneken

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2014

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

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 5, 2014

Member

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

$filesystem->mkdir($targetDir, 0777);
// We use a custom iterator to ignore VCS files
$filesystem->mirror($originDir, $targetDir, Finder::create()->ignoreDotFiles(false)->in($originDir));
}
}
}
if ($input->getOption('auto')) {
$output->writeln(sprintf('Assets were installed as <comment>%s</comment>.', $autoSymlinkFailed?'hard copies':'symbolic links'));

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 5, 2014

Member

I think I'd like even more information here, especially if the symlinks fail. Perhaps something like:

if ($autoSymlinkFailed) {
    $output->writeln('It doesn\'t look like your system supports symbolic links, so the assets were installed by copying them');
} else {
    $output->writeln('The assets were installed using symbolic links');
}
if (!file_exists($targetDir)) {
throw new IOException(sprintf('Symbolic link "%s" is created but appears to be broken.', $targetDir), 0, null, $targetDir);
}

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 5, 2014

Member

We need someone on a VM that has a Windows host machine (and where the project files are shared with the host machine) to try this and see if this properly catches the failure: #11297 (comment)

@Richtermeister

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2014

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

@andrerom

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2014

@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

This comment has been minimized.

Copy link
Member

commented Jul 7, 2014

@pborreli Can you try it on your Windows machine?

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

This comment has been minimized.

Copy link
@stof

stof Jul 7, 2014

Member

the iterator seems unused

@pborreli

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2014

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

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

This comment has been minimized.

Copy link
@pborreli

pborreli Jul 7, 2014

Contributor

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

This comment has been minimized.

Copy link
@rvanginneken

rvanginneken Jul 7, 2014

Author Contributor

yes , seem to have looked over that one

@pborreli

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2014

@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™️ 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

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2014

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2014

@andrerom Yes, happy to join the testers.

@pborreli

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2014

@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

This comment has been minimized.

Copy link
Member

commented Jul 13, 2014

@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

This comment has been minimized.

Copy link

commented Sep 7, 2014

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

This comment has been minimized.

Copy link
Member

commented Sep 12, 2014

@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

This comment has been minimized.

Copy link

commented Sep 12, 2014

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

This comment has been minimized.

Copy link
Member

commented Sep 12, 2014

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

@stof

This comment has been minimized.

Copy link
Member

commented Sep 12, 2014

👍

@Tobion

This comment has been minimized.

Copy link
Member

commented Sep 12, 2014

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

This comment has been minimized.

Copy link
Member

commented Sep 12, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2014

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

@Swader

This comment has been minimized.

Copy link

commented Sep 13, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2014

@Swader great then 👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 16, 2014

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

@andrerom

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2014

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

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 16, 2014

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

@andrerom

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2014

ok

@weaverryan

This comment has been minimized.

Copy link
Member

commented Sep 16, 2014

@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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2014

@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

This comment has been minimized.

Copy link
Member

commented Sep 16, 2014

@rvanginneken Perfect, thanks :)

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 22, 2014

👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 22, 2014

Thank you @rvanginneken.

@fabpot fabpot closed this Sep 22, 2014

fabpot added a commit that referenced this pull request Sep 22, 2014

feature #11312 Make assets:install smarter with symlinks (Roy Van Gin…
…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

andrerom added a commit to ezsystems/ezpublish-kernel that referenced this pull request Oct 11, 2014

Make ezpublish:legacy:assets_install & ezpublish:legacybundles:instal…
…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)

andrerom added a commit to ezsystems/ezpublish-kernel that referenced this pull request Oct 11, 2014

Make ezpublish:legacy:assets_install & ezpublish:legacybundles:instal…
…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)

andrerom added a commit to ezsystems/ezpublish-kernel that referenced this pull request Oct 12, 2014

Make ezpublish:legacy:assets_install & ezpublish:legacybundles:instal…
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.