Ocramius / PackageVersions

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

Probable race condition in post-install hooks #110

Closed Jean85 closed 4 years ago

Jean85 commented 5 years ago

This package is a a sub-sub dependency in sentry/sentry, and users are reporting issues while using the Laravel Sentry integration (sentry/sentry-laravel).

Users are reporting issues while installing: https://github.com/getsentry/sentry-laravel/issues/274 & https://github.com/getsentry/sentry-laravel/issues/284

As I already said in https://github.com/getsentry/sentry-laravel/issues/274#issuecomment-536901862:

The problem seems to be that the Illuminate\Foundation\ComposerScripts::postAutoloadDump is running BEFORE the ocramius/package-versions, so when that package is used, it's NOT ready yet.

Is there any way to raise the priority of this script, since it has no dependencies other that having the composer.lock ready and readable?

Ocramius commented 5 years ago

I don't think priority is supported by composer

MrMadClown commented 5 years ago

Any Idea how to solve this Issue? We run into this problem in our CI environment, where we start without a vendor directory every time. Which basically is the solution from getsentry/sentry-laravel#274.

Ocramius commented 5 years ago

@MrMadClown no clue on how to solve this, sorry

MrMadClown commented 5 years ago

Is this a composer issue then? In my case, the install runs fine, it crashes after the composer Install. Once we start the PHPUnit tests.

Start PHPUnit tests
PHPUnit 7.5.16 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.23
Configuration: /var/www/html/phpunit.xml

In FallbackVersions.php line 44:

Required package "sentry/sentry" is not installed: cannot detect its version
Ocramius commented 5 years ago

Yeah, that means that the script wasn't run at all, but also that "sentry/sentry" isn't in your installed.json or composer.lock (used by FallbackVersions.php)

MrMadClown commented 5 years ago

Okay, scripts not running, that's something I can start to look into. The sentry/sentry package is in the composer.lock file and also gets installed. That's was throwing me off.

Ocramius commented 5 years ago

Can you please open a separate issue? I think this one is unrelated with what's being asked here (hooks conflicting between laravel and this library - possibly upstream composer issue)

Jean85 commented 5 years ago

Reported upstream with https://github.com/composer/composer/issues/8399

Ocramius commented 5 years ago

@Jean85 note that FallbackVersions should still be able to detect sentry/sentry, so I think something else is also failing somewhere.

I suggest debugging FallbackVersions behavior in your environment, @MrMadClown.

Jean85 commented 5 years ago

In FallbackVersions, composer.lock surely is located because we do not see any 'PackageVersions could not locate yourcomposer.locklocation. exception.

This means that somehow Sentry is not shown there, but that shouldn't happen!

Ocramius commented 5 years ago

Note that installed.json takes priority, if located: maybe it's a dev-dependency?

Jean85 commented 5 years ago

The original issues arise when running composer require sentry/sentry-laravel, so it's not a dev dependency. Is it possible that installed.json is written later in respect to the post-autoload-dump hook?

Ocramius commented 5 years ago

That could be, yes. Reverting #95 could expose that.

Jean85 commented 4 years ago

I've probably found the root cause, which was much simpler that I was expecting:

https://github.com/getsentry/sentry-laravel/issues/284#issuecomment-552804087

82 is included in 1.5.0 only, which means that it's restricted to PHP ^7.3. The end user was probably using an older PHP version?

clxmstaab commented 4 years ago

yeah.. we had several issues because of fixes in this lib are only available in pretty recent php-versions

Ocramius commented 4 years ago

Can this be reproduced in isolation or in integration test? Otherwise I might close as "can't fix" here

Jean85 commented 4 years ago

If I'll take a stab at backporting #82 to 1.4, would you be open to tag it as 1.4.1?

Ocramius commented 4 years ago

Yep: should I create the 1.4.x branch?

Jean85 commented 4 years ago

Yes thank you, so I'll target the PR accordingly.

Ocramius commented 4 years ago

1.4.x is open :-)