Installs plugins and dependencies before installation of other packages #3082

Closed
wants to merge 3 commits into
from

Projects

None yet
@francoispluchino
Contributor
Q A
Bug fix? yes (installation plugin order)
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #2972, #2879, #2878, #1121, #3078
License MIT

This feature allows installing plugins and their dependencies before restarting the installation of other packages in the project. It allows the plugins to be added directly in the project and must not be installed in global.

With this feature it is for example possible to create a plugin adding automatically the repositories, without that Composer throws an error when the first installation.

It does not change the original behavior of the install, but at the same time, fixes a "strange" behavior of the order of the installation of plugins and their dependencies.

The dependencies plugins must be installed before the plugin, then the plugin must be installed, and then the rest of depedencies can be installed.

Test example:

{
    "repositories": [
        {
            "type": "package",
            "package": [
                { "name": "pkg", "version": "1.0.0" },
                { "name": "pkg2", "version": "1.0.0" },
                { "name": "inst", "version": "1.0.0", "type": "composer-plugin" },
                { "name": "inst2", "version": "1.0.0", "type": "composer-plugin", "require": { "pkg2": "*" } }
            ]
        }
    ],
    "require": {
        "pkg": "1.0.0",
        "inst": "1.0.0",
        "inst2": "1.0.0"
    }
}

The order should be:

Installing inst (1.0.0)
Installing pkg2 (1.0.0)
Installing inst2 (1.0.0)
Installing pkg (1.0.0)

and not (actually):

Installing inst (1.0.0)
Installing pkg (1.0.0)
Installing pkg2 (1.0.0)
Installing inst2 (1.0.0)
@nsams
nsams commented Jun 30, 2014

+1

This makes many things possible using plugins.

@francoispluchino
Contributor

@Seldaek It would be really cool to have this improvement (and correction) for the plugin system, do you think this PR could be merged?

@francoispluchino francoispluchino referenced this pull request in fxpio/composer-asset-plugin Jul 2, 2014
Closed

Run the plugin in "project" mode #7

@cebe cebe and 1 other commented on an outdated diff Jul 2, 2014
src/Composer/Installer.php
@@ -338,11 +343,13 @@ protected function doInstall($localRepo, $installedRepo, $platformRepo, $aliases
$this->package->getDevRequires()
);
- $this->io->write('<info>Loading composer repositories with package information</info>');
+ if ($onlyPlugins) {
@cebe
cebe Jul 2, 2014 Contributor

shouldn't this be !$onlyPlugins to keep the behavior that was before?

Same applies below for all other messages. maybe even show messages in both cases.

@francoispluchino
francoispluchino Jul 2, 2014 Contributor

@cebe No, the installer running 2 pass, and to keep the compatibility, the informations must be hidden in the second pass.

@francoispluchino
francoispluchino Jul 2, 2014 Contributor

@cebe because this information is to appear before the installation of plugins and their dependencies.

@francoispluchino
Contributor

@Seldaek, @naderman Do you think that this PR has a chance to be merged quickly?

This PR has no a break compatibility, the project with or without plugin retains the same running, but allows the plugins to be able to carry more thing in "project" mode.

@naderman
Member
naderman commented Jul 8, 2014

Doesn't this entirely break the installation of plugins that depend on regular composer packages?

@francoispluchino
Contributor

@naderman no break for this: the new version of the plugins installation, allows install the regular (composer) dependencies of plugins before the installation of plugins.

@naderman
Member
naderman commented Jul 8, 2014

Guess this looks ok to me, @Seldaek what do you think about the multiple pool implementation of this? I can't really think of a better alternative either though.

@francoispluchino
Contributor

@naderman Thanks!

Only the conditions for the display of informations are unnecessary, but it helps to keep the old display (and therefore, not having duplicate information in the console).

I made 4 attempts before proposing this PR, and this one, is the most optimized and uses maximum of the existing code. This allowed me to see the inner workings of Solver and Pool, and it's really a nice work!

@francoispluchino
Contributor

Continuing to add new features to my plugin, I find that I come quickly to the limitations of the plugin system, same with this PR.

With the PR #3116, I propose to add new events to the Installer, allowing the plugins to do much more thing, while maintaining compatibility with the existing plugin system.

@naderman, @Seldaek Can you look the issue #3116, and tell me if I can add the feature to this PR? Or create another PR.

@francoispluchino
Contributor

@naderman, @Seldaek Do you have any news for this PR?

@naderman
Member

Going to merge this I think, but that should certainly stay a separate PR.

@francoispluchino
Contributor

@naderman Ok, I do not edit this PR in this case.

@francoispluchino
Contributor

@Seldaek Is it possible to merge this PR, if it suits you?

@Seldaek Seldaek added this to the Backwards Compatible milestone Jul 19, 2014
@Seldaek Seldaek added the Feature label Jul 19, 2014
@nsams
nsams commented Jul 24, 2014

@Seldaek sorry to annoy, could please comment on this if you plan to merge it.

@francoispluchino
Contributor

@nsams I spoke with @Seldaek last week, and he prefers to fix urgent problems and take the time to look a little closer this PR.

@yousefcisco

Is anything happening with this PR? Does anyone know when it will be merged?

@damiandennis

+1 for merging

@francoispluchino
Contributor

@Seldaek, @naderman Is it possible to merge this PR?

@samdark
samdark commented Sep 3, 2014

+1 for merging it as well. Big plans on the feature...

@pmoust
pmoust commented Sep 3, 2014

👍 For merge.
@francoispluchino good job implementing this and documenting this PR.

@francoispluchino
Contributor

@Seldaek Can you merge this PR? I know you are very busy right now, but there are more and more people that wishing to use Composer with this fix of the bug for order of plugins installation. Thanking you in advance.

@echernyavskiy

+1 on merging this - this is much needed bug-fix.

@johonunu

+1 for merging

@PommeVerte

+1 for merging

@janisto
janisto commented Sep 17, 2014

+1 for merging

@schmunk42
Contributor

👍

@githubjeka

please, please, please

@Seldaek
Member
Seldaek commented Sep 18, 2014

I plan on looking at this this afternoon after I deploy some important changes to packagist that require some careful checks.. Please stop nagging it just makes me waste time deleting emails. I know very well it's important to you.

@schmunk42
Contributor

< no-nag-mode >
@Seldaek We understand this. You may also lock an issue to prevent all the +1 messages. I saw this in several other repos and as long as there's a short statement why an issue is locked and how the devs would like to proceed with it, eg. it will be merged it if there's time or not be merged, because of other issues - everybody should be fine with it.
< /no-nag-mode >

@iGrog
iGrog commented Sep 18, 2014

+1 for merging

@Seldaek
Member
Seldaek commented Sep 18, 2014

So after looking at the code and trying it out a bit I have to say I don't think this can be merged as is. It adds a second pass of solving which, while a bit faster than the first thanks to memoizatoin, is still a lot of wasted cycles for most users that won't need this bower stuff.

I made some test runs with/without patch and it seems to be taking almost twice as long to solve dependencies with the patch. That's just not something we can inflict on all users.

Technically the new pool and a second pass isn't a bad idea to allow what you want, but I think we need to be smarter and only do this when actually needed. Since you are loading packages that don't exist in the composer ecosystem, we can't simply detect the plugin to be installed is of a special type and then run a second pass, since the first pass will fail to resolve.

As far as I see it leaves us with only the option of having a flag in the root composer.json enabling this PR's behavior. But that's not really ideal either since if you require a package that requires the bower plugin and some bower packages, unless you know about it and add the flag in your composer.json it would fail to install.

The only nice-ish way I can think of is recommending that people install the plugin globally if they want bower support.

Any other ideas? Did anyone else actually try it and saw different performance results?

@francoispluchino
Contributor

@Seldaek check if no plugin is required, and if that's the case, do not make a second pass.

@githubjeka

Composer with this global plugin performed an order of magnitude longer. Many people complain about it.

@echernyavskiy

@Seldaek have been using composer with this patch in production for about 3 weeks now - haven't noticed much difference performance-wise or time-wise. Haven't run any tests though.

@Seldaek
Member
Seldaek commented Sep 18, 2014

@githubjeka but you don't HAVE to use the plugin. It's slower if you do but if I merge this it'll be slower for everyone.

@francoispluchino ok that's one idea, but what about the second pool that creates any missing package magically? We would need to fail the solving if any of those fake packages is present and no plugin is there to handle it in a second pass. I think/hope this can be done without creating any side effects.

@francoispluchino
Contributor

Personally didn't notice much change on speed, but it is true that making a second pass analysis requires more processing time at the Solver, but never in the Repositories because the 'whatProvides' caches the research.

@githubjeka

@francoispluchino yiisoft/yii2#5060 (comment) and next to that.
And yiisoft/yii2#5083 (comment)
and many other places on forums... People wait for a few minutes.

@francoispluchino
Contributor

@githubjeka This relates to the plugin you use that create the VCS repositories to the fly and imports the packages. Unfortunately the first retrieval of the each package informations is long, but it's the operation of the VCS repository of Composer (after Composer retrieves the package informations in cache). This does not apply to this issue.

@francoispluchino
Contributor

@Seldaek I forgot to mention that the PR is not specific to my plugin, but solves the problem with the installation of the dependencies required by plugins.

Currently, a plugin that requires a dependency will be installed before its dependence, because the dependence will be installed with the rest of the dependencies.

This PR fixe precisely the problem. Of course, I remain concerned about the performance, and if another implementation allows to solve the problem, I'm interested.

@cebe cebe and 1 other commented on an outdated diff Sep 18, 2014
src/Composer/DependencyResolver/PluginPool.php
+ * @param bool $mustMatchName Whether the name of returned packages
+ * must match the given name
+ *
+ * @return array A set of packages
+ */
+ public function whatProvides($name, LinkConstraintInterface $constraint = null, $mustMatchName = false)
+ {
+ $packages = parent::whatProvides($name, $constraint, $mustMatchName);
+
+ if (0 === count($packages) && !isset($this->packageByName[$name])) {
+ $key = ((int) $mustMatchName);
+ $candidate = new Package($name, '0.0.0.0', '0');
+ $candidate->setId($this->id++);
+ $this->packages[$this->id - 2] = $candidate;
+ $this->providerCache[$name][$key] = array($candidate);
+ $packages[] = $candidate;
@cebe
cebe Sep 18, 2014 Contributor

@francoispluchino can you explain why the creation of dummy packages is needed?

@francoispluchino
francoispluchino Sep 18, 2014 Contributor

To install the plugins even if a package is not present, and after, rescan all packages for check the validation. Allowing the plugins to be able to create a VCS Repository, for example.

@cebe
cebe Sep 18, 2014 Contributor

If in the first run we do only install plugins and their dependencies, shouldn't it be okay if some other packages do not exist? I am still digging into how this all works so I am not aware of how the process works as a whole :)

@francoispluchino
francoispluchino Sep 18, 2014 Contributor

I have not tested this case.

@cebe
cebe Sep 18, 2014 Contributor

Okay, looks like this is needed because we can only filter by package type (to only install plugins) when we have resolved the package to a specific version and got its composer.json....

@cebe
Contributor
cebe commented Sep 18, 2014

Doesn't the change in #2107 including the added method become obsolete with this PR?

@schmunk42
Contributor

While the following idea is also not a perfect solution, it may address the problem from a different side...

What's about introducing an switch for install and update like --plugins-first which is enabled by default only when running composer create-project?

For sure it would take longer for everybody on the first install (with create-project only), but install and update would have the same performance.

Another downside would be, that eg. virtual bower packages would not be handled by the updated plugin, unless you use the above option. But this could be detected I think and the user could get a warning message, like the notice for a self-update.

Addon

While trying to understand the problem, would you agree to the following extended example?

{
    "repositories": [
        {
            "type": "package",
            "package": [
                { "name": "foo/plugin-pkg", "version": "1.0.0" },
                { "name": "pkg", "version": "1.0.0" },
                { "name": "pkg2", "version": "1.0.0" },
                { "name": "inst", "version": "1.0.0", "type": "composer-plugin" },
                { "name": "inst2", "version": "1.0.0", "type": "composer-plugin", "require": { "pkg2": "*" } }
            ]
        }
    ],
    "require": {
        "foo/plugin-pkg": "1.0.0",
        "pkg": "1.0.0",
        "inst": "1.0.0",
        "inst2": "1.0.0"
    }
}

On of the plugins should handle the virtual foo/plugin-pkg...

Installing inst (1.0.0)
Installing pkg2 (1.0.0) <-- package must not depend on a plugin
Installing inst2 (1.0.0)
Installing pkg (1.0.0) <-- package position does not really matter anymore
Installing foo/plugin-pkg (1.0.0)

This is the use-case which requires plugins to be installed before any other packages, except if they are a dependency of a plugin.
[ ] Yes [ ] No, _______

@francoispluchino
Contributor

@schmunk42 And if a dependency require a new dependency plugin on the update job? Considering that the plugin also require an dependency for working correctly. In this case, an error will be thrown.

@cebe
Contributor
cebe commented Sep 18, 2014

@Seldaek at first, thanks for taking the time to review this PR and comment on it!

[...] is still a lot of wasted cycles for most users that won't need this bower stuff.

Please note that this is not solely about bower and npm packages but about the plugin structure of composer itself. It fixes similar issues like #1147 completely and makes #2107 obsolete(more specifically this line and function: https://github.com/composer/composer/blob/master/src/Composer/Installer.php#L485), which is an optimization that can be done afterwards.

I made some test runs with/without patch and it seems to be taking almost twice as long to solve dependencies with the patch. That's just not something we can inflict on all users.

I do not really see how the new approach is slower.
Did some benchmarking using a plain installation:

  1. steps:

    Results:

    • With composer from this PR: plugin install: 33sec., package install: 90sec, overall: 123sec.
    • 2nd run (directly after first): 12 sec.
    • With composer build d79f2b0fd33ee9b89f3d9f1969f43dc3d570a33a 2014-09-10 15:11:05:
      • 1st run: 121 sec.
      • 2nd run (directly after first): 10 sec.
  2. Another benchmark with a very simple composer.json (also removed ~/.composer dir):

    {
        "require": {
                "cebe/markdown": "0.9.*"
        }
    }
    • composer build d79f2b0fd33ee9b89f3d9f1969f43dc3d570a33a 2014-09-10 15:11:05: 9 sec.
    • second run: 2,5 sec.
    • composer from this PR: 9 sec.
    • second run: 2 sec.

I do not think the difference is relevant compared to the improvement this brings.

Technically the new pool and a second pass isn't a bad idea to allow what you want, but I think we need to be smarter and only do this when actually needed.

We can not detect plugins until we have resolved the whole set of dependencies because of the way composer works. I have just looked into the Installer::doInstall() method and tried to move the filtering by plugin to be done before the dependency resolver but this is not possible.
We only know whether a package is a plugin or not when we have a fixed version to install because we need to check its composer.json. With the current design of composer a package may be a plugin in one version but no plugin in the other so we can not avoid this as far as I see.

The only nice-ish way I can think of is recommending that people install the plugin globally if they want bower support.

This is the current situation and it leads to weird behavior when people forget to install the plugin globally.
I personally would not see this as a feature request but a bug in the installation of plugin dependencies. The order of installation is just wrong. (The part with the dummy packages is a feature of course :) )

We would need to fail the solving if any of those fake packages is present and no plugin is there to handle it in a second pass. I think/hope this can be done without creating any side effects.

In the second pass no fake packages are generated so they fail as expected.

Sorry for the long comment, hope it helps and thanks again for your time!

@francoispluchino
Contributor

@cebe Thank you for that explanation, however, can you perform a test with a very long list of dependency? just for feeling the time spent by the two analyzes.

@Seldaek Thank you for taking the time to analyze my work.

@cebe
Contributor
cebe commented Sep 18, 2014

How long should a "very long list of dependency" be? When benchmarking this we should focus on dependency complexities that are commonly used. Also the longer your list of dependencies the more constraints you need to make them actually work together. As far as I see the time spent in the resolver is hardly the critical factor when running composer install or update but rather the time it spends on network requests fetching package information and cloning the repos...

@francoispluchino
Contributor

@cebe About it, I just created issues on the performance of remote file system and the solver (see #3282 and #3283).

@stof
Contributor
stof commented Sep 19, 2014

Actually, the case where it will impact the solver perf is when requiring a package with lots of matching versions. try to depend on several Symfony components with a ~2.0 constraint for instance

@discordier discordier referenced this pull request in contao-community-alliance/composer-client Sep 20, 2014
Open

NPM / Bower asset management in Contao / Composer #210

@francoispluchino
Contributor

@stof I'll do performance tests this afternoon for different frameworks / projects that use many dependencies (symfony, symfony standard edition, laravel, yii2, sylius, orocrm). And we measure more clearly the impact of this fix.

@francoispluchino
Contributor

Test protocol:

Each framework/project is:

  • tested 6 times with install command and the --prefer-dist option
  • tested 6 times with update command and the --prefer-dist option after each install command

Each first install/update is not used to calculate the average, for put the downloaded files in cache.

Here are the tests results:

Install command without this patch:

Project Solver Do Install
Symfony 5.82s 28.84s
Laravel 12.16s 33.52s
Yii2 60.73s NA
Symfony Standard Edition 35.30s 70.05s
Sylius 104.34s 182.06s
OroCRM 178.05s 272.28s

Install command with this patch:

Project Solver pass 1 Solver pass 2 Solver Do Install
Symfony 5.88s 0.41s 6.29s 28.88s
Laravel 12.21s 6.13s 18.34s 42.76s
Yii2 51.88s 2.88s 54.76s NA
Symfony Standard Edition 37.98s 29.99s 67.97s 102.40s
Sylius 92.23s 85.94s 178.17s 257.56s
OroCRM 174.79s 171.63s 346.42s 442.49s

Update command without this patch:

Project Solver Do Install
Symfony 7.39s 7.40s
Laravel 19.11s 19.59s
Yii2 NA NA
Symfony Standard Edition 34.49s 35.57s
Sylius 96.85s 100.38s
OroCRM 325.78s 334.94s

Update command with this patch:

Project Solver pass 1 Solver pass 2 Solver (sum) Do Install
Symfony 7.27s 1.38s 8.65s 8.67s
Laravel 18.89s 13.29s 32.18s 33.18s
Yii2 NA NA NA NA
Symfony Standard Edition 35.64s 29.79s 65.43s 67.62s
Sylius 109.60s 90.85s 200.45s 207.27s
OroCRM 316.90s 312.28s 629.18s 654.50s

Difference for Install command:

Project Solver
Symfony +8%
Laravel +51%
Yii2 -10%
Symfony Standard Edition +92%
Sylius +70%
OroCRM +94%

Difference for Update command:

Project Solver
Symfony +17%
Laravel +68%
Yii2 NA
Symfony Standard Edition +89%
Sylius +106%
OroCRM +91%

And the results show a very high loss of performance on projects that use a lot of dependencies.

Concerning Yii2:

  • I don't know why, but I have best results to each time,
  • Because I don't have the SVN, and a dependency using the SVN is used, I get an error at the install.

@cebe For the moment, I'm agree with @Seldaek, it's not recommended to merge this PR.

@francoispluchino
Contributor

@Seldaek Is that this proposal may suit you:

It is not specific to my plugin, but will allow a project to require a package with X version must be required in "global", and if the dependency is not present, Composer will install and load the global dependencies before running the Solver.

To do this, we should add a "require-global" section to the Package (but only for root package). In this case, the Installer checks the presence of the dependencies in the "require-global" section, and if at least one dependency is present, Composer then launches an checks. Of course, if no global dependency is required, Composer will skip this job. Finally, if the dependency is not present, the Composer will install before.

Unfortunately, this proposal does not fix the problem of the installation order of the plugin dependencies, but it helps to get a job in 2 passes without sacrificing performance. This is somehow, a extended feature (and more general) of your proposition of flag in the root package.

@cebe
Contributor
cebe commented Sep 21, 2014

The idea of "require-global" sounds very interesting to me.

@schmunk42
Contributor

Would it be possible to have different versions of a package when using "require-global"?

@francoispluchino
Contributor

@schmunk42 I don't think it is possible. The system would only check if the global dependencies is present, and it will install the dependencies (or try update). In the same way that if you were doing it manually. Of course, if there is no matched version, an exception will be thrown.

It looks more like an automated execution of these commands:

  • composer global require vendor/package-name:X.Y.Z@dev
  • composer install/update
@PommeVerte

I haven't looked at the code and such but I figured I would throw an idea out just in case:
It sound to me that with how things are set up it's going to boil down to functionality vs execution time. On one side this PR is a lifesaver for most who need it, on the other the performance blow means it definitely shouldn't be imposed on those who don't.

So what about specifying in the root composer.json file that composer should use this PR as is (well... adapted to this use case)? By default composer would function as it currently does (without the PR).
It seems to me that it would make more sense to enable it on a per project basis, rather than via a command line param.
Obviously this would imply the need for this flag to be properly bubbled up in the package documentation as to not end up flagging it off when a sub-sub-sub-dependency requires it on.

I'm not sure if my understanding of the problem at hand is accurate but I figured it wouldn't hurt to suggest something.

@francoispluchino
Contributor

@PommeVerte I think your proposal is the same as @Seldaek (see comment), no?

@schmunk42
Contributor

@francoispluchino I was just wondering about this fact, but it seems logical to me how you explained it.
But shouldn't it be considered to install multiple versions of global packages simultaneously? There may be cases, where you'll end up switching the plugin version all the time. Which would not be the case, if you could install the plugin as a non-global package.

@PommeVerte

@francoispluchino duh I somehow missed that.
The thing is, every project has an installation mention so I would expect such dependencies would be properly bubbled up in each project (and rarely forgotten). Even in the event it were forgotten or no documentation made mention of it. How negative an effect would it be to receive an clear explanatory error during install (I'm assuming we could detect that a dependency requires the PR when loading it).
It might take a while for the error to to popup but it would only happen once in what is already an unlikely scenario.

@francoispluchino
Contributor

Sorry to make proposals, then others, then come back to the original proposal, but it's true that I love my first proposal (to add the new capabilities of a plugin, while correcting the problem install order of dependencies).

However, this PR lack of optimization. BUT, I have may be a functional solution to optimize this initial proposal, and even, for projects that require plugins (unless if the plugin is updated or is installed for the first time, in this case there will be 2 pass). It's still not the perfect solution, but I will you send performance testing for this approach (if I can do it...).

Regarding the proposal for the "require-global" section, it might be interesting to create a issue, what do you think, is it useful (I have a proof of concept for this feature)?

Otherwise, do you have another proposals to solve this problem in a single pass?

@schmunk42
Contributor

@francoispluchino Absolutely no need to say sorry!
While I also think that your first proposal is the right way and that the current implementation is buggy^W optimized for the usage without plugins ;)
But other issues like performance should also be considered.
Using a global installation brings up other problems I think, but maybe they are not as problematic as I think atm.

@pmoust
pmoust commented Sep 21, 2014

I 'd much rather have this merged as is and then get the implementation flawless rather than dumping the PR without another attempt to attack the same issue in another PR.

While the solver takes a considerable impact in execution time, the essence of this PR is a problem-solver.

I'd take a 50-60s hit on install/update over a mess of dependencies any time.

@francoispluchino francoispluchino referenced this pull request in yiisoft/yii2 Sep 22, 2014
Closed

Issues with the new asset management #5083

4 of 4 tasks complete
@francoispluchino
Contributor

Progress status of this PR

After a big headache to understand how to operate the Solver SAT, I finally found a solution that should please you (I think)! Otherwise, @Seldaek and @naderman did a excellent work.

Regarding this new approach:

  1. There are more the creation of MockPackage by the Pool
  2. Backwards compatibility with all current projects, that is, 1 single pass is performed by the Solver, even in case of problems
  3. The order of installation of plugins and their dependencies is always corrected
  4. If a plugin is to install/update, and there is no problem, the plugin will be installed (with their dependencies) before others packages in one single pass
  5. If no plugin to install/update (even if there are already plugins in vendor directory), and that there is at least one problem, an exception will be thrown immediately with the errors list (no second pass)
  6. If a plugin is to install/update, and that there is at least one problem, (package does not exist for example), a second passage will be required (the information appears in the console)

Generally, all installations are done in one single pass, except when it has at least one error and a plugin must to be installed/updated.

For example, with the plugin fxp/composer-asset-plugin, if one of the dependencies of the project require this plugin and that there has at least one reference to an asset (NPM/Bower), Composer will install the plugin, and it will execute a second pass to install all dependencies. On the other hand, for the update, and given that the plugin is already installed, one single pass will be done in all cases.

Regarding performance:

Project Install Update
Symfony +0% +0%
Laravel +0% +0%
Yii2 ° +0% +0%
Symfony Standard Edition +0% +0%
Sylius +0% +0%
OroCRM +0% +0%

° Yii2 with plugin fxp/composer-asset-plugin installed in global mode

Given that @Seldaek has made corrections to the plugins installation order, I must still merge my work with the latest version of Composer. I'll let you know.

@francoispluchino
Contributor

The new version of this PR is available.

@francoispluchino
Contributor

@Seldaek Does is this new approach is right for you? And in this case, can it be merged?

@armab
armab commented Sep 26, 2014

Subscribed on this PR
+1 to support this feature, makes life easier

@Seldaek
Member
Seldaek commented Sep 30, 2014

@francoispluchino the latest version of the PR definitely looks better to me, much less invasive and brute-force-ish than before. If you can rebase it I'll try to review a bit more in depth when I find a moment and try it out here as well.

@armab for future reference to avoid sending notifications to everyone, there is a subscribe button on the bottom of the side-bar you can use to subscribe :)

@francoispluchino
Contributor

@Seldaek It's done.

@armab
armab commented Oct 13, 2014

@naderman @Seldaek can you guys please check again this PR?

I believe all Yii2 framework community will be thankful for this feature.

@tristanlins tristanlins referenced this pull request in contao-community-alliance/composer-plugin Oct 17, 2014
Closed

Write journal file #25

@francoispluchino
Contributor

@Seldaek Do you think this PR can be merged in the coming weeks? This feature is really expected for many users.

Thanking you in advance.

@samdark samdark referenced this pull request in fxpio/composer-asset-plugin Oct 30, 2014
Closed

Ability to use the Nodejs with NPM and Bower #60

@francoispluchino
Contributor

@Seldaek Is this this PR will be merged? I have many requests to run my plugin in "project" mode, but it needs this PR to do this.

I understand that you are overworked currently, but what is we must change for merged this PR?

@francoispluchino
Contributor

@Seldaek Would you like that I add an option allowing to a plugin getting a new analysis by the Installer? This will keep a complete backward compatibility for projects using plugins.

For this PR, currently, when a plugin is installed / updated, and that there is at least one error, a second analysis is performed. With this solution, if no plugin has this option enabled, there will be no second pass by the Installer, and the error will be displayed immediately.

I propose to add the options in the extra section of composer.json file of the plugin:

  • installer-restart-on-exception (bool, false by default): to restart the analysis only if there is an error
  • installer-restart-forced (bool, false by default): for always restart the analysis, with or without error

Description:

  1. If there is at least one plugin with the option installer-restart-forced enabled, then, a second analysis will always be performed, with or without error in the Installer
  2. If there is no plugin with the option installer-restart-forced enabled, but that there is the least one plugin with the option installer-restart-on-exception enabled, then, a second analysis will be made only in case of error
  3. If there is no plugin with these options, then, there will be no second pass (current behavior)

I think with this solution, we maintain full compatibility and identical behavior (by default), we fix the order of installation of the dependencies of a plugin, and we add the capability for the plugins, to restart a second analysis if they need it, with a manual activation in their composer.json file.

With this option, is that okay for you?

@tristanlins
Contributor

Sounds useful 👍

@cebe
Contributor
cebe commented Oct 31, 2014

@francoispluchino imo this should work out of the box and by default, I do not see why we need these options. Or are these options to be set in the composer.json of the plugin? not sure if and how that should work.

@francoispluchino
Contributor

@cebe Only in the composer.json file of the plugin. This is a flag to indicate to Composer that the plugin requires a second pass for the Solver.

For it is true that if there is a plugin that is installed, but there is an error in the Solver, a second pass is performed, while it is not necessary. With this solution, this second pass is performed only if there is a plugin that request a new pass.

@cebe
Contributor
cebe commented Oct 31, 2014

Does the plugin get installed regardless of the error in the first pass?

@francoispluchino
Contributor

@cebe Of course, in the first analysis, Composer will:

  • analyze the dependencies
  • check if a plugin must be installed (with these dependencies)
  • check if a plugin needs a second analysis (new proposal),
  • install the plugins (if any),
  • storing the errors (if any),
  • activate the plugins,
  • perform a second analysis (if required) or throw an exception (if any)

I think it's also possible to use the event dispatcher, instead of the config in composer.json file.

@cebe
Contributor
cebe commented Oct 31, 2014

cool, thanks for clarification!

@francoispluchino
Contributor

After analyse, it's not possible to use the event dispatcher, because we absolutely must install the plugins. It's not necessary if we use the extra section of the config.

@francoispluchino
Contributor

I rebased this PR with the last version. And I added a condition for restart the analysis of dependencies, with only one option in extra section: installer-restart.

This option can take as value:

  • false (default value): no restart of the Installer. If there is an exception, no dependencies or plugins will be installed
  • on-exception: indicates that the plugin requires a new analysis, only if the Solver has a list of problem
  • true: indicates that the plugin requires a new analysis for each install/update of the plugin (concerns only the plugin, no the other operations)

In verbose mode, the list of plugins requiring a new analysis is displayed.

Example of plugin composer.json:

{
    "name": "acme/foobar-plugin",
    "type": "composer-plugin",
    "extra": {
        "class": "Acme\\Composer\\FoobarPlugin",
        "installer-restart": "on-exception",
        "branch-alias": {
            "dev-master": "1.0-dev"
        }
    }
}
@francoispluchino
Contributor

@Seldaek @naderman What about of this PR?

Given that now, this PR keeps the current behavior for all plugins, but allows to the plugins, to restart the analysis of dependencies.

@schmunk42
Contributor

@Seldaek @naderman Please have a look at this PR and give us a statement how you want to proceed with it, IMHO it's crucial to many projects and it affects the way you have to setup your application.

Eg. if you forget to install a plugin globally, you'll get weird errors during app installation.

@Seldaek Seldaek commented on the diff Dec 8, 2014
src/Composer/Installer.php
@@ -380,7 +393,9 @@ protected function doInstall($localRepo, $installedRepo, $platformRepo, $aliases
}
if ($this->update) {
- $this->io->write('<info>Updating dependencies'.($withDevReqs ? ' (including require-dev)' : '').'</info>');
+ if ($delayValidation || (!$delayValidation && $this->io->isVerbose())) {
@Seldaek
Seldaek Dec 8, 2014 Member

This could just be $delayValidation || $this->io->isVerbose() and same below line 449/471

@Seldaek
Seldaek Dec 8, 2014 Member

On the other hand.. can't we just output this twice when a plugin restarts the whole thing? It would keep the code cleaner and I don't think it's so wrong from the user point of view?

@francoispluchino
francoispluchino Dec 8, 2014 Contributor

We can display the same information when the plugin restarts, but I added this condition to avoid having too much information in duplicate.

@Seldaek
Seldaek Dec 8, 2014 Member

Do you have easy instructions for me to try a restarting plugin? Just so
I can see how it looks for the user etc.

@francoispluchino
francoispluchino Dec 9, 2014 Contributor

Create a plugin with the installer-restart option:

{
    "name": "acme/foobar-plugin",
    "type": "composer-plugin",
    "extra": {
        "class": "Acme\\Composer\\FoobarPlugin",
        "installer-restart": true
    }
}

Read this comment for more explanation on the installer-restart option.

@francoispluchino
francoispluchino Dec 9, 2014 Contributor

I made you a test plugin:

{
    "require": {
        "fxp/composer-demo-restart-plugin": "dev-master"
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "git@github.com:francoispluchino/composer-demo-restart-plugin.git"
        }
    ]
}
@Seldaek Seldaek commented on an outdated diff Dec 8, 2014
src/Composer/Installer.php
@@ -490,6 +510,7 @@ protected function doInstall($localRepo, $installedRepo, $platformRepo, $aliases
$operations = $this->movePluginsToFront($operations);
$operations = $this->moveUninstallsToFront($operations);
+ $operations = $this->keepOnlyPluginsAndDependencies($operations, $delayValidation);
@Seldaek
Seldaek Dec 8, 2014 Member

Conditionally calling this if ($delayValidation) would be clearer than calling it always and having a huge if in the method IMO. Same for validateOperations above at 494

@Seldaek
Member
Seldaek commented Dec 8, 2014

This looks much better now thanks @francoispluchino. No more performance impact unless needed which is great. Apart from the few coding style things which disturb me a bit (this class is already quite huge to digest so let's try not to make it worse than necessary:) I would say it's mergeable but would still like to hear from @naderman.

@francoispluchino
Contributor

@Seldaek I just made the correction forif ($delayValidation).

@naderman
Member
naderman commented Dec 9, 2014

Maybe this PR should actually include some tests? I'm a little hesitant on the solver changes, isn't there a chance this could result in a plugin getting installed which the second run then uninstalls cause it wasn't supposed to be installed in the first place?

@naderman
Member
naderman commented Dec 9, 2014

Hm I suppose not since the Problem gets generated either way, just not sure if the failure information is always correct then if you make a decision anyway and don't correctly compute the rest of the dependencies.

@francoispluchino
Contributor

@naderman The first install keep only the plugins and their dependencies (defined by the Solver, but it do not throw an exception if any problem is found - if the option installer-restart has the value true or on-exception).

In the second pass, the Solver finds that the plugins X, Y, Z are already installed, and therefore, there is no operation. Furthermore the Solver mechanism is not changed.

@naderman
Member
naderman commented Dec 9, 2014

Well you do call revertLast() which may in fact revert a previous problem decision that your new code made, as that is a loop in which a decision is not necessarily made on every iteration, so the entire logic there is unclear to me.

@naderman naderman commented on the diff Dec 9, 2014
src/Composer/DependencyResolver/Solver.php
@@ -89,6 +92,13 @@ private function makeAssertionRuleDecisions()
$problem->addRule($conflict);
$this->disableProblem($rule);
$this->problems[] = $problem;
+
+ if ($this->skipException) {
+ $cLiterals = $conflict->getLiterals();
+ $cLiteral = abs($cLiterals[0]);
+ $this->decisions->revertLast();
@naderman
naderman Dec 9, 2014 Member

Which decision do you expect this to be? The one that resulted in a conflict? That is not necessarily the case. It could also simply be another iteration of this loop and a decision made by the line below?

@francoispluchino
francoispluchino Dec 9, 2014 Contributor

This is to allow the Solver to continue the analysis even if there is an error. For example, when a package is not found. Without revertLast(), it's not possible to skirt the problem, but maybe you have a better solution.

@naderman
naderman Dec 9, 2014 Member

What I'm saying is that revertLast() may revert something entirely different from what you're expecting.

@francoispluchino
francoispluchino Dec 9, 2014 Contributor

Without this call, it does not work. Do you have a better solution?

@naderman
naderman Dec 9, 2014 Member

It does not work with the call either. No I do not. This is really just not what the solver is made for at all. While this may have worked in some case you tested it with, it is going to have entirely random effects and we can't possibly make this change.

@francoispluchino
francoispluchino Dec 9, 2014 Contributor

I did several tests, and I've always had the same behavior. Can you give me an example that could give a random behavior?

@naderman naderman commented on the diff Dec 9, 2014
src/Composer/DependencyResolver/Solver.php
@@ -89,6 +92,13 @@ private function makeAssertionRuleDecisions()
$problem->addRule($conflict);
$this->disableProblem($rule);
$this->problems[] = $problem;
+
+ if ($this->skipException) {
+ $cLiterals = $conflict->getLiterals();
+ $cLiteral = abs($cLiterals[0]);
@naderman
naderman Dec 9, 2014 Member

Why are you certain that the first element is the literal that you are interested in? Couldn't it also be [1]?

@francoispluchino
francoispluchino Dec 9, 2014 Contributor

Because it's the element that is the problem.

@naderman
naderman Dec 9, 2014 Member

Not necessarily. $conflict is simply the rule that lead to a decision that is now in conflict with deciding $literal. So you're just randomly picking the first element. If you are interested in the literal that is actually conflicting right now use $literal.

@francoispluchino
francoispluchino Dec 9, 2014 Contributor

To be more specific about its usefulness, This section allows to the Installer to continue the analysis of sub dependencies even if the package contains an error.

For example, when a package requires a plugin and a package that does not exist without the plugin. Without this section, it is not possible to add the plugin required by the package with an error.

@naderman
Member
naderman commented Dec 9, 2014

Why is it that you need to avoid solver exceptions at all if all you are trying to do is reordering the package instalaltion? You can't just let the solver decide something gets installed, let it make decisions based on that and then change it to not installed and let it make decision based on that, you'll end up with a chaos of conflicting decisions if you happen to call this with the wrong combination of dependencies.

@francoispluchino
Contributor

For example, with the plugin fxp/composer-asset-plugin, You can add dependencies that are not referenced in Packagist (or other repositories). So the Solver throws an problem (package not found).

If the plugin is added to a sub-dependence (or even lower), The je n'ai The Installer has not the list of plugins to install.

@stof
Contributor
stof commented Dec 9, 2014

@francoispluchino a much better way to solve the case of asset would be to define a repository providing these packages to the solver, rather than trying to make the solver behave in weird ways

@Seldaek
Member
Seldaek commented Dec 9, 2014

The problem is just a chicken and egg issue. The plugin can't provide
anything to the solver until it's installed, and it can't be installed
because the solver will fail to solve things as the asset packages are
missing when the plugin isn't there.

@francoispluchino
Contributor

@stof If we are forced to add repositories manually, the concept of the plugin is useless. The plugin automatically adds the required VCS repository, but the plugin must be present.

However, there are other cases of use, which requires that the analysis be performed with the installed plugins.

@francoispluchino
Contributor

@Seldaek This is exactly the problem. And if you have a better solution, I'm interested.

@Seldaek
Member
Seldaek commented Dec 9, 2014

I do see the problematic, but I also agree with @naderman that if this
is possibly making the solver unstable it's not acceptable as is.

I don't know a better way except using bower or npm directly instead of
hacking things into composer that don't really belong there.

@samdark
samdark commented Dec 9, 2014

@Seldaek bower/npm direct usage isn't really suitable in many cases. For example, we're using the plugin since we're wrapping widgets into packages. A widget is a set of JavaScript + CSS + PHP wrapper that expects certain version of JavaScript and CSS. Currently it's all fully automated, no nodeJS required to use it and sub-dependencies are supported. Everything's perfect except the fact that plugin has to be installed globally in advance.

Are there parts of solver not covered by tests? If so, which ones? If these will be covered so we'll be sure that solver is stable after changes, would the pull request be accepted?

@francoispluchino
Contributor

I also agree with @naderman, if it makes it unstable the Solver, this is not acceptable.

For my plugin, I think more and more to make a wrapper for NPM and Bower, but alas, and you know it, they also have disadvantages:

  • no lock file for Bower,
  • Bower only works with GIT
  • obligation to install an SSH key on the machine to work with the private repositories
  • obligation to have the different drivers installed

And if my plugin is increasingly used, it is precisely because, it is not necessary to install nodejs, npm and bower.

@schmunk42
Contributor

Isn't the question if plugins are allowed to provide virtual packages or "solutions" to the solver?
If so, they have to be treated in a prioritized way, somehow.

I don't know a better way except using bower or npm directly instead of
hacking things into composer that don't really belong there.

@Seldaek To me, the client-side packages and server-side packages are very closely linked. Isn't the task npm, bower and composer are doing merely the same - don't they just have different repo formats?

@schmunk42
Contributor

For my plugin, I think more and more to make a wrapper for NPM and Bower, ...

@francoispluchino I'd rather like to have a require-global option. At the moment you get weird errors if you're missing the plugin, your gets installed, but does not really work. If there an error message what's missing, it should be clear.

@Seldaek
Member
Seldaek commented Dec 9, 2014

One thing we could maybe do (just a thought, not sure if it's actually feasible without checking more in depth) to solve the error reporting bit is to support a new plugin type or some value in extra attributes that would let us display a message in the solver failure that the required plugin X provides new packages and installing it globally might fix the issue. That being said, I don't know if it's worth it considering that most people it seems don't read the error messages at all no matter how clear they are.

@francoispluchino
Contributor

@Seldaek That is why we must automate the plugin installation, even if there are errors for the other dependencies (not plugins and not dependencies of plugins), and relaunch the analysis with the installed plugins.

@francoispluchino
Contributor

@Seldaek The flag already exists in this PR, it's the extra.installer-restart option in composer.json.

@naderman
Member
naderman commented Dec 9, 2014

@samdark The test suite is still not entirely complete so yes. However none of the tests are being run with skipExceptions = true at all, as the tests don't contain any of these types of plugins. We'd need a whole additional test suite for all sorts of situations with this type of plugin.

@francoispluchino
Contributor

@naderman I'll add the unit tests.

@francoispluchino
Contributor

@naderman Tests added.

@francoispluchino
Contributor

@naderman Do the tests are right for you?

@bishopb
bishopb commented Jan 1, 2015

Neither npm nor bower know vendor/. Only composer does. Thus, some composer-based package needs to stitch together the vendor knowledge for npm and bower to digest. One such package is composer-npm-bridge. I've been using it for a while to resolve package.json, which includes bower, that then resolves bower.json. It works well.

But composer-npm-bridge requires npm. I like that composer-asset-plugin doesn't require external programs. Since composer, npm, and bower are all doing essentially the same thing, it seems reasonable that one package could emulate the others and that the dependency list could be specified in one place.

But with all respect, I think the plugin's design of npm-asset and bower-asset as "virtual" vendors is misguided. That design decision is at the root of this issue. And other future issues: when someone creates an "npm-asset" (or "bower-asset") packagist category, then this plugin will essentially stop working, without some patching to map around the new category.

@Seldaek, I don't think we should change fundamental composer behavior to suit a plugin. Instead @francoispluchino, I think the plugin should "work with the grain" of composer. In this specific scenario, composer-asset-plugin could scan a separate file (eg composer-assets.json) that lists asset dependencies, then doing its own work after install has completed.

@cebe
Contributor
cebe commented Jan 1, 2015

In this specific scenario, composer-asset-plugin could scan a separate file (eg composer-assets.json) that lists asset dependencies, then doing its own work after install has completed.

This is against what you claim before: "'work with the grain' of composer". If the plugin installs packages, these should be installed in the resolving process of composer and not afterwards and outside. You could use a completely separated tool for that. With the plugin we are able to specify dependencies on bower/npm assets and they get installed if matching constraints are found or we get an error if it is not possible to resolve the situation. A separate process can not fulfill the requirements here.

@bishopb
bishopb commented Jan 1, 2015

@cebe: In the use cases I've encountered (which I recognize does not encompass all use cases), front-end assets rarely conflict. Either (a) I deliberately choose packages that have compatible front-end assets or (b) handle the front-end entirely in the root package. For me, the bootstrapping requirement to "install npm and/or bower installers" trumps the requirement "install no dependencies if any of their assets conflict".

Holistic assertions like "This PHP package requires Laravel 5, jQuery UI 1, Bootstrap 3 SASS, and grunt-sass" are a noble goal. I fully support that goal. But the approach in this PR concerns me. Not because of the implementation, per se, but because it's doing something composer was never designed for: "virtual" packages.

If there's a plug-in to be made, it seems to me satis and/or packagist would be the place to house that plug-in, not composer. virtual/npm-* and virtual/bower-* would proxy through the existing plugin code to resolve/map npm and bower to dependency specs that composer already consumes. In this approach, everyone using composer immediately benefits, rather than just those who happen to stumble upon the plugin and add it to their composer.json. And, this approach avoids the squatting problem (someone taking the npm-asset or bower-asset vendor).

Bottom line: I would prefer to see this handled universally via packagist virtual "proxying". Alternatively, I'm happy managing back- and front-end dependencies separately, so long as I don't have to manually manage npm/bower installers. But not this PR. I don't see the value.

@samdark
samdark commented Jan 1, 2015

The use case we're talking about are "builders". PHP code that generates JavaScript, HTML and CSS code.

For example, based on validation rules in your PHP code it can generate validation rules for a given version of JavaScript form validation library.

The thing with dependencies is that JavaScript library can be updated independently from PHP one so everything will be broken. In this case an ability to have a dependency between PHP package and JS package is vital.

@cebe
Contributor
cebe commented Jan 1, 2015

I would prefer to see this handled universally via packagist virtual "proxying"

This is an option that would be technically possible but this means that we would have to integrate the bower/npm packages into composer for everyone which was not accepted as a solution by many people in discussions we had before we started using this plugin.

I have made a proof of concept for this some time ago:
https://github.com/cebe/composer-bower
some comparison/discussion:
cebe/composer-bower#5

@bishopb
bishopb commented Jan 1, 2015

@samdark Yes, I follow. Both approaches I'm advocating handle that, and neither require this PR.

Suppose PHP code builds Javascript that requires jQuery 2.1.2 and jQuery UI 1.10.4.

In the first approach, the one that works today but is not ideal, composer.json contains:

"require": { "eloquent/composer-npm-bridge": "~2" }

The package.json file contains:

"dependencies": { "jquery": "2.1.2", "jquery-ui": "1.10.4" }

Running composer install pulls down composer-npm-bridge 2.* into vendor, which runs post install the npm installer to get jQuery 2.1.2 and jQuery UI 1.10.4. One can independently change the versions by editing these separate files. And upstream can happily release new versions without impacting the code. If one later add a PHP package that uses npm to require jQuery 1.11.2 (which is incompatible with 2.1.2) the post install step will complain and can be resolved appropriately.

If packagist specifically or satis generally were to add the features from this plugin, which the second approach I advocate, then one only needs a single composer.json to specify all dependencies:

"require": {
    "eloquent/composer-npm-bridge": "~2",
    "virtual/npm-jquery": "2.1.2",
    "virtual/npm-jquery-ui": "1.10.4"
}

The plugin wants to specify the syntax like above. That syntax is fantastic! I love it. I want it. But I do not feel the composer plugin approach is working. It's asking composer to do things it wasn't designed to do. It's reserving "npm-asset" and "bower-asset" without any teeth to enforce the reservation. It only works for those who happen upon the plugin.

The PHP world needs what this plugin offers. But not this way. Let packagist handle the proxying. Let composer continue to operate as it does.


@cebe I believe a blending of your experiment and this plugin is the best road to follow. Packagist would expose npm and bower packages via a reserved virtual vendor. Upon a request for a specific package within that vendor (eg, virtual/npm-jquery), packagist (via the plugin code) would scan the corresponding npm (or bower) repository for matching versions. That dependency graph would be packaged in composer-expected format and returned (as the plugin does now), and cached for subsequent fetches.

This approach is one of on-demand proxying in Packagist proper, rather than a separate Packagist-like repository that bootstraps npm and bower registries.

Analysis: Existing packages continue to work unaffected. If a package author decides he needs an npm- or bower-held asset, he can look them up in Packagist and add them directly to his package's composer.json. He doesn't have to install a special plugin. He doesn't have to do a composer update to get the revised version with this new restart feature. We're throwing away none of the plugin's great code: just moving where the work is done. True, the Packagist farm is doing more work (because there are now lots more packages), but the work done is just "more of the same" and done "just in time".

@damiandennis

This plugin works great but I see the point being made, It sounds like a program needs to be made that can integrate composer, npm and bower into one package and check dependencies before running each of the package managers. I am not sure that however is even possible as it depends on how flexible each of the package managers. Also it does sound like a lot of work...

@schmunk42
Contributor

@bishopb

Either (a) I deliberately choose packages that have compatible front-end assets or (b) handle the front-end entirely in the root package. For me, the bootstrapping requirement to "install npm and/or bower installers" trumps the requirement "install no dependencies if any of their assets conflict".

(c) I install a bundle of PHP, CSS and JS files like described here and without any manual installation or setup.

That's my everyday use-case and I see no better solution than using the asset plugin, which does a wonderful job here.

It's asking composer to do things it wasn't designed to do.

That's why we have the possiblity to create plugins, you can never foresee all possible use-cases or new ideas.

It's reserving "npm-asset" and "bower-asset" without any teeth to enforce the reservation. It only works for those who happen upon the plugin.

I agree that this could be improved, ie. these paths could be made configurable like the paths, where the npm or bower packages are installed, see here for an example.

If you use the virtual packages, you also have to require the plugin, the problem is, that you have to install it globally at the moment, that's what this PR is trying to solve.

As a sidenote: You can create packages under any namespace, there's no reservation like the user- or org-name on GitHub.

True, the Packagist farm is doing more work (because there are now lots more packages), but the work done is just "more of the same" and done "just in time".

This looks problematic (kindly speaking) to me, see Module Counts, for a comparision of the numbers:

  • packagist 46480
  • bower 24855
  • npm 115298

You're suggesting to proxy over the threefold number of packages than currently hosted on packagist - while we are already experiencing a lag of several minutes when creating a new version - and moreover a very large percentage of npm and bower packages are not of interest for PHP projects.

This would totally clutter the packagist search and is no option IMHO.

@francoispluchino
Contributor

@bishopb

But with all respect, I think the plugin's design of npm-asset and bower-asset as "virtual" vendors is misguided. That design decision is at the root of this issue. And other future issues: when someone creates an "npm-asset" (or "bower-asset") packagist category, then this plugin will essentially stop working, without some patching to map around the new category.

I agree with you, and that's why I chose to prefix the asset dependency with TYPE-asset. Nobody will use this prefix, but it is true that it can be problematic.

Regarding the design problem: thank you for your constructive comments, and it's always interesting to have external advice. I will not repeat all of my approach to my plugin (because it is not the subject), but the main idea was to use the maximum the native components of Composer (Installer, Sovler, Locker, Commands, etc...) and simply transformed the package information for Composer.
Personally I would have liked the possibility to prefix the asset dependency with "@bower" and "@npm" for example. In this way, Composer knows that dependency is a virtual dependency and does not take into account in its installation (by the Solver).

I also had the possibility to put the asset dependencies in the "extra" section, but in this case, we do not use the native components of Composer.

Your proposal to use "virtual/npm-" and "virtual/bower-" has the same problem as my implementation.

Your proposal to use a proxy, was my initial research: technically, it is possible, but unfortunately, the analysis is extremely heavy, and therefore, the limitation of 5000 request/hours to Github API would soon exploded just NPM has over 80 000 packages).

@bishopb; @cebe, @samdark The debate around my plugin is very interesting, but it not concern this PR directly. Indeed, this PR is not specific for my plugibut it allows to solve the problem of installation order of dependencies of plugins, and allows to the plugins to do the same action whatever the chosen mode of installation (projet or global mode).

What makes me happy, is to see the interest aroused by this PR when there are more than 6 months, and that I was ready, for the new year, to close this PR to choose another solution (but significantly less native).

I think about the management of assets (javascript/stylesheet) for servers languages other than JS, is a very interesting and important topic, because with any Framework used, management of assets is simply bypassed. For example I use a lot Symfony2, and the recommendation is to not put the assets in Bundles, but it no offers a technical solution to easily handle this case. And for other Frameworks (ZF2, Laravel4, etc...), from my initial tests, it's the same problem, except for Yii2 (but that may have changed since).

Another point, it is not because a project is not initially designed to manage something, that this tool can not evolve, if that something is a recurring problem. The management of assets is a recurring and pervasive problem for any project that manage the back-end and front-end. Given that Composer manages the PHP dependencies, I think it would be interesting it can manage the assets (without for all that use my solution).

For finish, you find that the principle is cool, but not the implementation, because for you, the plugin asks Composer to do something it does not. In this regard I do not agree with you. Did you see the Doc and FAQs? (see How does work the plugin?).

@bishopb
bishopb commented Jan 2, 2015

@francoispluchino @schmunk42 I'm glad we're continuing to have this discussion, as I feel a comprehensive asset management solution would help the whole community. But, yes, we're off topic with this PR. Is there a mailing list, IRC channel, or other place we can move this larger conversation?

For the PR. I did read the plugin docs, and I did read the entire PR, and I know a little bit about the Composer code base. I am not an expert, though.

I am with @stof when he said "define a repository providing these packages to the solver, rather than trying to make the solver behave in weird ways". I would prefer that repository be up at Packagist. But, I am ok with the dependencies being in "extra", as I think you meant in your recent #3602 comment.

I am with @naderman when he said "This is really just not what the solver is made for at all" regarding the Solver.php change. I am following the logic, and it seems to work. But again I'm not an expert. I personally want to see more test coverage, because I do not want a "Composer debacle" (like the silly npm debacle). Some tests that I think are needed:

  • plugin-restart-always.test but with --no-dev
  • plugin-restart-always.test but with a/a in conflicting versions
  • plugin-restart-always.test but with a/a depending on b/b
  • plugin-restart-always.test but with a/a depending on b/b in conflicting versions
  • timing tests with real-world (ie "non-trivial") dependency graphs
@francoispluchino
Contributor

@bishopb Ok for the new tests, but can you be more precise for the last test ("timing tests with real-world (ie "non-trivial") dependency graphs")?

@bishopb
bishopb commented Jan 2, 2015

@francoispluchino The test cases have a maximum of three dependencies. The packages and applications I regularly work on have hundreds of dependencies, specified in cross-cutting fashion: A depends upon B and C; B and C both depend on D; and B and D depend upon E. Network overhead aside, the composer solver takes some time to process that dependency graph (call that time T_i). I would like to have a performance test that illustrates how much average additional time, if any, the restarts and other changes add to T_i.

@francoispluchino
Contributor

@bishopb For the cost of time, you can see this comment.

@schmunk42
Contributor

@francoispluchino Could your plugin detect if it was installed globally and if not, fail with a human friendly error mesage?

@francoispluchino
Contributor

@schmunk42 I think this is feasible with the property HOME directory. But for example, you added the plugin such as dependency of your project, and you ask your users to install the plugin in global mode.

At the first installation, the plugin in global mode is used, on the other hand, once the plugin is installed in project mode, it is the project plugin which is used (project plugins has a priority to global plugin).

So if I add a exception, I will break compatibility.

@cebe
Contributor
cebe commented Jan 10, 2015

So basically what this fixes is the first install, right?

@francoispluchino
Contributor

@cebe Right.

@bishopb
bishopb commented Jan 10, 2015

@cebe @francoispluchino Reaffirming that "first" appears to be the root issue, as mentioned in the require-global conversation. And also reaffirming what @schmunk42 said in that same conversation, paraphrased: require-global will not prevent fixing 3082, but will prevent first-time related errors`.

@francoispluchino
Contributor

@bishopb My proposal of require-global (see this comment) does not prevent an error if a global plugin is not installed. This could be possible only for the root Composer package (read the proposal).

Indeed, the sub dependencies indicating that they need a global plugin (via require-global) cannot be analyzed, because the problem is the same that this PR: that is, that Composer cannot know the require-global of the dependence, because, in this dependency, there will be a sub-dependency that Composer will not know, and therefore an error will thrown.

To correct the problem, we should in this case, performing a first analysis dedicated to require-global, and launch a second analysis to resolve dependencies.

For reminder, it was the first version of this PR, and performance is poor compared to the last version proposed (see this comment vs this comment).

It is true, that the require-global will allow for a complete project to avoid unnecessary error, but this PR is much more relevant, especially that this latest version does not change the performance for all current and future projects, but only for projects that require a plugin must be installed prior to analysis (and only for the first installation, not updates, il the plugin is not installed in global mode).

@Seldaek Seldaek modified the milestone: Later, Backwards Compatible Apr 29, 2015
@Marlinc
Marlinc commented May 25, 2015

Any news on this?

@kschu91 kschu91 added a commit to AOEpeople/CmsComposerInstallers that referenced this pull request Jul 10, 2015
@kschu91 kschu91 Force installer restart after installation. Otherwise it is not possi…
…ble to use inline branch alias.

See: composer/composer#3082
6d92a6f
@Seldaek Seldaek closed this Feb 21, 2016
@schmunk42
Contributor

@Seldaek Do you have more feedback why this is closed now? Is it just closed or solved by a commit?

@Seldaek
Member
Seldaek commented Feb 22, 2016

It's just not mergeable so closing to clean up. It's not solved and I doubt it is really solveable without relying on global pre-installed plugins as people have been doing until now.

@schmunk42
Contributor

Thank you for the info. Let me reference #3607 one more time.

It would be really great if we could get something like that, because I've seen so many bug reports where people were missing a global installation of the mentioned plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment