Ocramius / PackageVersions

:package: Composer addon to efficiently get installed packages' version numbers
MIT License
3.22k stars 67 forks source link

FallbackVersions uses this package lock which breaks version checks #116

Closed scaytrase closed 4 years ago

scaytrase commented 4 years ago

In some cases PackageVersions\Versions does not have version for package being checked:

These are different cases which exposes differently, but the root cause is FallbackVersions using this package composer.lock file. It does not filter out dev packages.

https://github.com/infection/infection/issues/1007

https://github.com/scaytrase/package-version-fallback-issue https://travis-ci.org/scaytrase/package-version-fallback-issue/builds/639467674

In linked travis build you can examine that infection/infection thinks it runs 0.13.4 (while required, locked and installed as 0.15.0, see logs). But this is minor issue. The major one is that infection thinks that it uses symfony/process 4.3.2 from this package lock file, but this is not true, since symfony/symfony is required and installed as ^5.0 and doesn't have called method anymore (see travis logs)

I didn't know what are the reasons of including this package lock into the FallbackVersions, but my suggested fix is to drop them out of there.

IMO I'd better get no package installed error and check possible fix ways instead of silently getting wrong version from fallback

Ocramius commented 4 years ago

Isn't this true also for the generated PackageVersions\Versions class?

In fact, asking for the version of foo/bar when foo/bar is not installed at all (and is instead replaced by something else) seems to be incorrect.

Can this be reproduced in isolation with a test?

See https://github.com/Ocramius/PackageVersions/blob/c4b240dfe1106a1306e321e9c9c691e1baadaae0/test/PackageVersionsTest/E2EInstallerTest.php#L98-L111

scaytrase commented 4 years ago

I'll try to dig into these tests. Actually problem raises only with packages that are in require-dev of this lib (or transtive to them)

In fact, asking for the version of foo/bar when foo/bar is not installed at all (and is instead replaced by something else) seems to be incorrect.

Yea, in this case I'd better get no package installed, than version from lock

Ocramius commented 4 years ago

You can go wild with repository definitions, as long as we isolate everything to a tiny example, heh.

On Mon, Jan 20, 2020, 19:13 Pavel Batanov notifications@github.com wrote:

I'll try to dig into these tests. Actually problem raises only with packages that are in require-dev of this lib (or transtive to them)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Ocramius/PackageVersions/issues/116?email_source=notifications&email_token=AABFVEHYGMOFPI3ST4KVQUDQ6XSTJA5CNFSM4KJEE4Q2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJNOUCQ#issuecomment-576383498, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEAZ42YVLIZUJXC6NX3Q6XSTJANCNFSM4KJEE4QQ .

scaytrase commented 4 years ago

Well, looks like I succeeded in reproducing this.

I've created two repos simulating dependencies https://github.com/scaytrase/package-versions-main-stub https://github.com/scaytrase/package-versions-sec-stub

I've created the test that uses them (commit is very dirty, tbh and contains some debug changes, like output)

https://github.com/Ocramius/PackageVersions/compare/master...scaytrase:bugfix/issue-116?expand=1

I've run travis for it.

https://travis-ci.org/scaytrase/PackageVersions/jobs/639786230

Installation output says

Package operations: 3 installs, 0 updates, 0 removals
  - Installing ocramius/package-versions (1.0.0): Downloading (100%)
  - Installing symfony/process (5.0): Downloading (100%)
  - Installing infection/infection (0.15.0): Downloading (100%)

Note the 5.0 for symfony/process

Then I compare it in test with what

echo \PackageVersions\Versions::getVersion('symfony/process')

gives me

And it fails

1) PackageVersionsTest\E2EInstallerTest::testInstallingPluginWithNoScriptsOverridesOriginalRequirements
Failed asserting that 'v4.4.2@b84501ad50adb72a94fb460a5b5c91f693e99c9b' matches PCRE pattern "/^v?5\..*\@[a-f0-9]*$/".
/home/travis/build/scaytrase/PackageVersions/test/PackageVersionsTest/E2EInstallerTest.php:357
/home/travis/build/scaytrase/PackageVersions/test/PackageVersionsTest/E2EInstallerTest.php:232

4.4.2 is not even present in my simulated repo (i have only two tags 4.0 and 5.0)

I can try to clean up things if you need them.

Ocramius commented 4 years ago

Can you send the branch you created in as a PR? I'd like to discuss it line-by-line, when I get to work on it

scaytrase commented 4 years ago

Sure #117 here it is. I hope draft state won't prevent it from discussion. just to mark it's dirty

soullivaneuh commented 4 years ago

Is there any workaround about this? I'm trying to run dependencies installation without script on Docker for cache perf improvements:

COPY composer* ./
COPY symfony.lock .
RUN composer install --prefer-dist --no-scripts --no-autoloader

# Heavier to ligther, for cache performance.
COPY src src
COPY src-dev src-dev
COPY assets assets
COPY templates templates
COPY translations translations
COPY bin bin
COPY config config
COPY public public
COPY .env .
COPY composer-scripts .
RUN composer dump-autoload
RUN composer run init-scripts
RUN composer run auto-scripts

But I ran onto the same issue, becaue of --no-scripts.

Ocramius commented 4 years ago

@soullivaneuh I suggest helping out on #117.

soullivaneuh commented 4 years ago

@Ocramius I was just asking if an alternative exist waiting this PR, but thanks, I'll take a look when I'll have the time. :+1:

Ocramius commented 4 years ago

Worked around by #146, which effectively removes the offending code.