From 4014c914ab488fd035f0f4b9c8f3b24bdcf55b24 Mon Sep 17 00:00:00 2001 From: Farhad Safarov Date: Tue, 21 Aug 2018 15:51:20 +0400 Subject: [PATCH 1/4] remove Github 404 retries --- src/Composer/Repository/Vcs/GitHubDriver.php | 27 ++++---------------- 1 file changed, 5 insertions(+), 22 deletions(-) diff --git a/src/Composer/Repository/Vcs/GitHubDriver.php b/src/Composer/Repository/Vcs/GitHubDriver.php index e150ccd10..fc256f82c 100644 --- a/src/Composer/Repository/Vcs/GitHubDriver.php +++ b/src/Composer/Repository/Vcs/GitHubDriver.php @@ -183,30 +183,13 @@ class GitHubDriver extends VcsDriver return $this->gitDriver->getFileContent($file, $identifier); } - $notFoundRetries = 2; - while ($notFoundRetries) { - try { - $resource = $this->getApiUrl() . '/repos/'.$this->owner.'/'.$this->repository.'/contents/' . $file . '?ref='.urlencode($identifier); - $resource = JsonFile::parseJson($this->getContents($resource)); - if (empty($resource['content']) || $resource['encoding'] !== 'base64' || !($content = base64_decode($resource['content']))) { - throw new \RuntimeException('Could not retrieve ' . $file . ' for '.$identifier); - } - - return $content; - } catch (TransportException $e) { - if (404 !== $e->getCode()) { - throw $e; - } - - // TODO should be removed when possible - // retry fetching if github returns a 404 since they happen randomly - $notFoundRetries--; - - return null; - } + $resource = $this->getApiUrl() . '/repos/'.$this->owner.'/'.$this->repository.'/contents/' . $file . '?ref='.urlencode($identifier); + $resource = JsonFile::parseJson($this->getContents($resource)); + if (empty($resource['content']) || $resource['encoding'] !== 'base64' || !($content = base64_decode($resource['content']))) { + throw new \RuntimeException('Could not retrieve ' . $file . ' for '.$identifier); } - return null; + return $content; } /** From 33341130a93add87c8c264ba3608e982600338ca Mon Sep 17 00:00:00 2001 From: Pierre du Plessis Date: Mon, 27 Aug 2018 09:13:52 +0200 Subject: [PATCH 2/4] Fix typo in variable name in GitHubDriver --- src/Composer/Repository/Vcs/GitHubDriver.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Composer/Repository/Vcs/GitHubDriver.php b/src/Composer/Repository/Vcs/GitHubDriver.php index e150ccd10..53ace8de7 100644 --- a/src/Composer/Repository/Vcs/GitHubDriver.php +++ b/src/Composer/Repository/Vcs/GitHubDriver.php @@ -378,7 +378,7 @@ class GitHubDriver extends VcsDriver return $this->attemptCloneFallback(); } - $rateLimited = $githubUtil->isRateLimited($e->getHeaders()); + $rateLimited = $gitHubUtil->isRateLimited($e->getHeaders()); if (!$this->io->hasAuthentication($this->originUrl)) { if (!$this->io->isInteractive()) { @@ -392,7 +392,7 @@ class GitHubDriver extends VcsDriver } if ($rateLimited) { - $rateLimit = $githubUtil->getRateLimit($e->getHeaders()); + $rateLimit = $gitHubUtil->getRateLimit($e->getHeaders()); $this->io->writeError(sprintf( 'GitHub API limit (%d calls/hr) is exhausted. You are already authorized so you have to wait until %s before doing more requests', $rateLimit['limit'], From e5b948c683e98ed2618431456ad447d0d7c8d209 Mon Sep 17 00:00:00 2001 From: Christophe Coevoet Date: Mon, 9 Jul 2018 22:07:12 +0200 Subject: [PATCH 3/4] Refactor the handling of conflict rules in the solver Conflict rules are not added in the solver based on the packages loaded in the solver by require rules, instead of loading remote metadata for them. This has 2 benefits: - it reduces the number of conflict rules in the solver in case of conflict rules targetting packages which are not required - it fixes the behavior of replaces, which is meant to conflict with all versions of the replaced package, without introducing a performance regression (this behavior was changed when optimizing composer in the past). --- src/Composer/DependencyResolver/Pool.php | 4 +- .../DependencyResolver/RuleSetGenerator.php | 86 +++++++++++++------ 2 files changed, 62 insertions(+), 28 deletions(-) diff --git a/src/Composer/DependencyResolver/Pool.php b/src/Composer/DependencyResolver/Pool.php index 3dc6d90be..085aaa7bf 100644 --- a/src/Composer/DependencyResolver/Pool.php +++ b/src/Composer/DependencyResolver/Pool.php @@ -317,12 +317,12 @@ class Pool implements \Countable * Checks if the package matches the given constraint directly or through * provided or replaced packages * - * @param array|PackageInterface $candidate + * @param PackageInterface $candidate * @param string $name Name of the package to be matched * @param ConstraintInterface $constraint The constraint to verify * @return int One of the MATCH* constants of this class or 0 if there is no match */ - private function match($candidate, $name, ConstraintInterface $constraint = null, $bypassFilters) + public function match($candidate, $name, ConstraintInterface $constraint = null, $bypassFilters) { $candidateName = $candidate->getName(); $candidateVersion = $candidate->getVersion(); diff --git a/src/Composer/DependencyResolver/RuleSetGenerator.php b/src/Composer/DependencyResolver/RuleSetGenerator.php index c534be958..60617ba43 100644 --- a/src/Composer/DependencyResolver/RuleSetGenerator.php +++ b/src/Composer/DependencyResolver/RuleSetGenerator.php @@ -28,6 +28,9 @@ class RuleSetGenerator protected $installedMap; protected $whitelistedMap; protected $addedMap; + protected $conflictAddedMap; + protected $addedPackages; + protected $addedPackagesByNames; public function __construct(PolicyInterface $policy, Pool $pool) { @@ -185,6 +188,7 @@ class RuleSetGenerator $workQueue->enqueue($package); while (!$workQueue->isEmpty()) { + /** @var PackageInterface $package */ $package = $workQueue->dequeue(); if (isset($this->addedMap[$package->id])) { continue; @@ -192,6 +196,11 @@ class RuleSetGenerator $this->addedMap[$package->id] = true; + $this->addedPackages[] = $package; + foreach ($package->getNames() as $name) { + $this->addedPackagesByNames[$name][] = $package; + } + foreach ($package->getRequires() as $link) { if ($ignorePlatformReqs && preg_match(PlatformRepository::PLATFORM_PACKAGE_REGEX, $link->getTarget())) { continue; @@ -206,32 +215,6 @@ class RuleSetGenerator } } - foreach ($package->getConflicts() as $link) { - $possibleConflicts = $this->pool->whatProvides($link->getTarget(), $link->getConstraint()); - - foreach ($possibleConflicts as $conflict) { - $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRule2Literals($package, $conflict, Rule::RULE_PACKAGE_CONFLICT, $link)); - } - } - - // check obsoletes and implicit obsoletes of a package - $isInstalled = isset($this->installedMap[$package->id]); - - foreach ($package->getReplaces() as $link) { - $obsoleteProviders = $this->pool->whatProvides($link->getTarget(), $link->getConstraint()); - - foreach ($obsoleteProviders as $provider) { - if ($provider === $package) { - continue; - } - - if (!$this->obsoleteImpossibleForAlias($package, $provider)) { - $reason = $isInstalled ? Rule::RULE_INSTALLED_PACKAGE_OBSOLETES : Rule::RULE_PACKAGE_OBSOLETES; - $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRule2Literals($package, $provider, $reason, $link)); - } - } - } - $packageName = $package->getName(); $obsoleteProviders = $this->pool->whatProvides($packageName, null); @@ -250,6 +233,49 @@ class RuleSetGenerator } } + protected function addConflictRules() + { + /** @var PackageInterface $package */ + foreach ($this->addedPackages as $package) { + foreach ($package->getConflicts() as $link) { + if (!isset($this->addedPackagesByNames[$link->getTarget()])) { + continue; + } + + /** @var PackageInterface $possibleConflict */ + foreach ($this->addedPackagesByNames[$link->getTarget()] as $possibleConflict) { + $conflictMatch = $this->pool->match($possibleConflict, $link->getTarget(), $link->getConstraint(), true); + + if ($conflictMatch === Pool::MATCH || $conflictMatch === Pool::MATCH_REPLACE) { + $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRule2Literals($package, $possibleConflict, Rule::RULE_PACKAGE_CONFLICT, $link)); + } + + } + } + + // check obsoletes and implicit obsoletes of a package + $isInstalled = isset($this->installedMap[$package->id]); + + foreach ($package->getReplaces() as $link) { + if (!isset($this->addedPackagesByNames[$link->getTarget()])) { + continue; + } + + /** @var PackageInterface $possibleConflict */ + foreach ($this->addedPackagesByNames[$link->getTarget()] as $provider) { + if ($provider === $package) { + continue; + } + + if (!$this->obsoleteImpossibleForAlias($package, $provider)) { + $reason = $isInstalled ? Rule::RULE_INSTALLED_PACKAGE_OBSOLETES : Rule::RULE_PACKAGE_OBSOLETES; + $this->addRule(RuleSet::TYPE_PACKAGE, $this->createRule2Literals($package, $provider, $reason, $link)); + } + } + } + } + } + protected function obsoleteImpossibleForAlias($package, $provider) { $packageIsAlias = $package instanceof AliasPackage; @@ -327,12 +353,20 @@ class RuleSetGenerator $this->pool->setWhitelist($this->whitelistedMap); $this->addedMap = array(); + $this->conflictAddedMap = array(); + $this->addedPackages = array(); + $this->addedPackagesByNames = array(); foreach ($this->installedMap as $package) { $this->addRulesForPackage($package, $ignorePlatformReqs); } $this->addRulesForJobs($ignorePlatformReqs); + $this->addConflictRules(); + + // Remove references to packages + $this->addedPackages = $this->addedPackagesByNames = null; + return $this->rules; } } From 8c3898aa576643fb4238141ad94c2f5a6e1e74ad Mon Sep 17 00:00:00 2001 From: Christophe Coevoet Date: Mon, 9 Jul 2018 22:51:23 +0200 Subject: [PATCH 4/4] Update tests for replace conflicts This reverts the test changes done in b4698568d2 to the original tests added in 1425bb7fc3. --- ...-installed-when-installing-from-lock.test} | 12 +++--- ...aced-packages-should-not-be-installed.test | 24 +++++++++++ ...kages-wrong-version-install-from-lock.test | 41 ------------------- 3 files changed, 29 insertions(+), 48 deletions(-) rename tests/Composer/Test/Fixtures/installer/{replaced-packages-wrong-version-install.test => replaced-packages-should-not-be-installed-when-installing-from-lock.test} (85%) create mode 100644 tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed.test delete mode 100644 tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install-from-lock.test diff --git a/tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install.test b/tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed-when-installing-from-lock.test similarity index 85% rename from tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install.test rename to tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed-when-installing-from-lock.test index 8971f8069..947d96b28 100644 --- a/tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install.test +++ b/tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed-when-installing-from-lock.test @@ -1,5 +1,5 @@ --TEST-- -Requiring a replaced package in a version, that is not provided by the replacing package, should install correctly (although that is not a very smart idea) +Requiring a replaced package in a version, that is not provided by the replacing package, should result in a conflict, when installing from lock --COMPOSER-- { "repositories": [ @@ -17,9 +17,7 @@ Requiring a replaced package in a version, that is not provided by the replacing "foo/replaced": "2.0.0" } } ---RUN-- -install ---EXPECT-LOCK-- +--LOCK-- { "packages": [ { "name": "foo/original", "version": "1.0.0", "replace": {"foo/replaced": "1.0.0"}, "type": "library" }, @@ -34,8 +32,8 @@ install "platform": [], "platform-dev": [] } +--RUN-- +install --EXPECT-EXIT-CODE-- -0 +2 --EXPECT-- -Installing foo/original (1.0.0) -Installing foo/replaced (2.0.0) diff --git a/tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed.test b/tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed.test new file mode 100644 index 000000000..0d1ea7701 --- /dev/null +++ b/tests/Composer/Test/Fixtures/installer/replaced-packages-should-not-be-installed.test @@ -0,0 +1,24 @@ +--TEST-- +Requiring a replaced package in a version, that is not provided by the replacing package, should result in a conflict +--COMPOSER-- +{ + "repositories": [ + { + "type": "package", + "package": [ + { "name": "foo/original", "version": "1.0.0", "replace": {"foo/replaced": "1.0.0"} }, + { "name": "foo/replaced", "version": "1.0.0" }, + { "name": "foo/replaced", "version": "2.0.0" } + ] + } + ], + "require": { + "foo/original": "1.0.0", + "foo/replaced": "2.0.0" + } +} +--RUN-- +install +--EXPECT-EXIT-CODE-- +2 +--EXPECT-- diff --git a/tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install-from-lock.test b/tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install-from-lock.test deleted file mode 100644 index 2721772ae..000000000 --- a/tests/Composer/Test/Fixtures/installer/replaced-packages-wrong-version-install-from-lock.test +++ /dev/null @@ -1,41 +0,0 @@ ---TEST-- -Requiring a replaced package in a version, that is not provided by the replacing package, should install correctly (although that is not a very smart idea) also when installing from lock ---COMPOSER-- -{ - "repositories": [ - { - "type": "package", - "package": [ - { "name": "foo/original", "version": "1.0.0", "replace": {"foo/replaced": "1.0.0"} }, - { "name": "foo/replaced", "version": "1.0.0" }, - { "name": "foo/replaced", "version": "2.0.0" } - ] - } - ], - "require": { - "foo/original": "1.0.0", - "foo/replaced": "2.0.0" - } -} ---LOCK-- -{ - "packages": [ - { "name": "foo/original", "version": "1.0.0", "replace": {"foo/replaced": "1.0.0"}, "type": "library" }, - { "name": "foo/replaced", "version": "2.0.0", "type": "library" } - ], - "packages-dev": [], - "aliases": [], - "minimum-stability": "stable", - "stability-flags": {}, - "prefer-stable": false, - "prefer-lowest": false, - "platform": [], - "platform-dev": [] -} ---RUN-- -install ---EXPECT-EXIT-CODE-- -0 ---EXPECT-- -Installing foo/original (1.0.0) -Installing foo/replaced (2.0.0)