-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
draft support for Pickle in Composer #3897
base: main
Are you sure you want to change the base?
Conversation
@@ -345,7 +345,6 @@ private function findFileWithExtension($class, $ext) | |||
{ | |||
// PSR-4 lookup | |||
$logicalPathPsr4 = strtr($class, '\\', DIRECTORY_SEPARATOR) . $ext; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be reverted
…ICKLE_PATH (just to ease dev for now)
Made most of the changes you pointed at, except the tmp dir one and the zip downloader |
I'm really not sure this is a good idea. We discussed in #2898 (comment) why having an ExtensionInstaller is a bad idea, and outlined how this should be implemented instead. The ExtensionInstaller installs dependencies globally which is very different from how the rest of composer behaves. It does not follow the per-project behavior of composer. If you use the ExtensionInstaller in one project it impacts all other projects on the same machine too, but you will never be informed that you broke the dependencies of the other projects. This needs to be handled much more explicitly and transparent for the user, as is outlined in the comment I linked to. |
@naderman huh. Also keeping in mind that multiple php support is coming. That and FPM Pool allows easy per project extensions managements, or fcgi on windows. So what do you suggest now? |
Yes I do want it to be integrated, just in the way outlined in that other PR, not as an extension installer. |
protected $pickle = 'pickle'; | ||
protected $process; | ||
/** | ||
* Initializes library installer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not the library installer
@naderman Ok, after discussions on IRC, makes sense for now. I will try to post a summary :) Will update the patch accordingly, this PR was a draft to start this exact discussion, so no worry. I am not sure where to start to skip extension during install step 1 and skip non extension during install step 2, somehow an ExtensionInstaller is still required, but do not have to be called if '--install-extensions' is not used. Or? |
how are available extensions known by Composer ? Does pecl.php.net support the Composer protocol to expose packages ? Does it require a special repository implementation ? Or does it need to wait for pickle-web ? |
No, it won't. Or I don't see why composer should do it. Mind to enlighten
|
@stof yes, see http://news.php.net/php.internals/85814 and we need a list anyway. Which one is bundled in which major version. They have to be handled differently. |
Also it is not composer's job to keep that list nor to deal with but pickle, composer does nothing about extensions but check if they are loaded. |
How far off would a PECL repository be from the PEAR repository? |
@slbmeh I do not get the question but let me try to answer based on a guess :) Pickle does not use pear, at all. It does not need package.xml but can handle package with only package.xml but no composer.json (live conversion). |
Also, I would love feedback about the patch (not CS pls, I will fix them) and if it is how you like to have it. I can go with a plugin instead for the meantime, as this is a blocker for now. |
@pierrejoye sorry I had the context in my head. That was a question building off of @stof's question about discovering extensions in regards to a special repository. I believe the PECL and PEAR repositories are rather similar and may be able to work with the existing PEAR stuff. |
|
||
protected function createTempDir() | ||
{ | ||
$lockfile = tempnam($this->cacheDir, 'pickle'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this create a file every time you generate a new directory name?
@slbmeh please check what Pickle is, it could be a good start. And no, we are not going to use pear for that, that's exactly the goal, to do not use pear anymore as it is a pain to use as an extension developer. |
@pierrejoye I'm aware of Pickle. I'm not suggesting anything about Pickle itself. I thought the question was about how will composer know about extensions. My reference to pear was about https://github.com/composer/composer/blob/master/src/Composer/Repository/PearRepository.php, I don't think it's too far of a stretch to make that work with pecl.php.net so you don't have to duplicate metadata. |
@pierrejoye the PlatformRepository providing the info about the platform itself needs to be able to know the version of installed extensions, otherwise you cannot use the version in the constraint. |
@slbmeh the PearRepository is a huge performance killer, because the PEAR protocol is just not adapter for Composer needs, required a huge number of requests to fetch the necessary data. |
OK, sorry but still no :) pecl is by far not the place anymore where ppl releases their extensions. And pickleweb (packagist for extensions) instead. Bringing the simplicity
|
@slbmeh pickle works with pecl.php.net transparently and out of th box, even if a release has no composer.json. Same for the web hooks on registration or release tag. @stof pickle ensures that the version is set correctly. It cannot be released if not. I also started a discussion on internals to fix the core extensions versions. |
@stof Will look at the PlaformInfo part, it could be possible to get it to know the installed extensions. I have to check to do not duplicate the installed version management. Not sure how yet. Waiting on @Seldaek and @naderman to comment on this patch, where it is done and how, then I will investigate further about these things. |
A couple of things we support in pickle and not sure how to make them work smoothly when called from composer. Old releases of an extension may not have composer.json (or will most not have), composer will fail on these release. What pickle does is to check for package(2).xml and convert on the fly to generate a usable composer.json. Absence of package.xml will be supported as well, composer.json being created from the src (README, header, etc). It is already partially done for the convert command. Ideas for the composer part? Or we may simply require it when used from composer, that will help extensions developers to get to add composer.json. |
Well composer doesn't really need the composer.json if the pickle repo provides the metadata. It is needed however if we want to load a github repo of an ext as a vcs repository in composer, so yes if people add it eventually it'd be great. |
We will enforce it for any new release. But the question is for older If not, and the repo (source, VCS, whatever) does not have composer, it may It also brings me back to the main point. What do you think about the draft
|
if (!$this->installExtensions && $operation->getPackage()->getType() == 'extension') { | ||
$this->io->writeError('<warning>This package has extensions dependencies, run composer install --install-extensons</warning>'); | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should maybe support 'update' operations too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an extension is loaded it cannot be updated. That's the tricky part. It
is also why we somehow need to be able to run pickle against a different
php than the one running pickle. Or use -n but then... :)
On May 5, 2015 5:11 PM, "Jordi Boggiano" notifications@github.com wrote:
In src/Composer/Installer.php
#3897 (comment):@@ -523,6 +525,10 @@ protected function doInstall($localRepo, $installedRepo, $platformRepo, $aliases
foreach ($operations as $operation) {
// collect suggestions
if ('install' === $operation->getJobType()) {
if (!$this->installExtensions && $operation->getPackage()->getType() == 'extension') {
$this->io->writeError('<warning>This package has extensions dependencies, run composer install --install-extensons</warning>');
continue;
}
Should maybe support 'update' operations too
—
Reply to this email directly or view it on GitHub
https://github.com/composer/composer/pull/3897/files#r29656783.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any news? |
@Seldaek Do you need any help to test and close this PR? |
bump |
Draft support for Pickle (https://github.com/FriendsOfPHP/pickle) in Composer.
Spent more time to dig composer code than actually implement that but at least it works to install exts from within a project.
Example project and extension composer.json:
https://gist.github.com/pierrejoye/1bbc2bdcec5887e60490
If you like to test with more extensions:
git clone myext
cd myext
pickle convert
Add the dep to your project. If you do not want to fork an ext, you can create a dep vs a zip file and use the artifact repository, it should work.
Still many things to solve but at least there is something to play with now
Update:
According to our discussions, it is now a two steps process to install extensions, to make sure users understand that installing extensions may affect many projects:
First, the usual command:
composer install
composer will tell you to run
composer install --install-extensions
if the project has extensions dependencies