Fix support for symlinks on Windows. #6213
Conversation
| @@ -76,40 +79,79 @@ public function download(PackageInterface $package, $path, $output = true) | |||
| $this->filesystem->removeDirectory($path); | |||
|
|
|||
| if ($output) { | |||
| $this->io->writeError(sprintf( | |||
| $this->io->write(sprintf( | |||
There was a problem hiding this comment.
this is a no-go. Feedback given to humans goes to the second pipe (named STDERR in PHP), not the first one
There was a problem hiding this comment.
Fixed. Had no clue... having "error" there was really confusing me.
| $this->io->writeError(sprintf(' Symlinked from %s', $url), false); | ||
| $linked = true; | ||
| } | ||
| catch (IOException $e) { |
There was a problem hiding this comment.
should be on the previous line
There was a problem hiding this comment.
Fixed. My IDE is using coding standards from other projects...
| if (!$linked && Platform::isWindows()) { | ||
| // Use command linke symlinking as a fallback. | ||
| try { | ||
| $return_var = 0; |
There was a problem hiding this comment.
Should use camelCase.
And you don't need to initialize it here, as it is a by-ref variable.
And $exitCode is a better name for what it does
There was a problem hiding this comment.
return_var is from PHP's exec documentation itself.
By ref does not guarantee that these variables are really set in the callee. I only do that to get the IDE to give them a type automatically. Should do no harm.
There was a problem hiding this comment.
The name $return_var is up to you entirely, it's just an example in the docs. The function will set the value by ref no matter what the variable is called.
There was a problem hiding this comment.
The function will set the value by ref no matter what the variable is called.
Sure. It was just a mater of consistency. Whatever, already changed it.
|
Just as a final thought. Even though this patch fixes symlinks on Windows, I would NOT recommend at all their usage. I.e. if you copy a symlink using the windows explorer, it will be ignored and copied as an empty folder. Or some tools (such as AppVeyor's artifact packages) will create Zip's where the files are duplicated and the symlinks removed. All-in-all, the best approach is to have composer use the packages in their original local path: |
I'm not really sure what issue you are describing with this, but the simple solution would just be to disable symlinking when building cross-platform artifacts. |
With "portable" I am not talking about moving this from windows to linux (or the other way round). I mean that Junctions always use absolute paths. So if I build an artifact using composer on one machine, CI system, whatever, then I will only be able to use it on another system that has exactly the same folder structure. It cannot be moved around. Plus all the other issues with symlinks themselves.
This means that these artifacts will have duplicate code, not a big deal, but can be an issue on big projects. |
| } | ||
|
|
||
| if (!$linked && Platform::isWindows()) { | ||
| // Use command linke symlinking as a fallback. |
|
As far as I understand, this doesn't fix much as symlinks or junctions are equally broken, and symlinks are generally not working anyway since most people don't run elevated shells, so I'd rather not merge it and risk a regression if it's not improving things much. If I'm mistaken let me know and we can revisit the issue. |
Fix for discussion on:
#6174
The symlink function from PHP in Windows is totally broken when using relative paths. This pull requests uses several fallback mechanisms before using a junction.
The problems with junctions is that they are not portable, making artifacts built with composer unusable for deployments.