CI tools moved to special composer.json #1391
Conversation
kukulich
force-pushed
the
ci
branch
8 times, most recently
from
6536578 to
8b8a643
Compare
Ocramius
left a comment
Ocramius
left a comment
There was a problem hiding this comment.
PHP Fatal error: Uncaught OutOfBoundsException: Package "vimeo/psalm" is not installed in /home/runner/work/BetterReflection/BetterReflection/vendor/composer/InstalledVersions.php:182
Welp, I may be able to patch around this in upstream 😬
| @@ -0,0 +1,30 @@ | |||
| { | |||
There was a problem hiding this comment.
Does renovate need adjustments here too? 🤔
There was a problem hiding this comment.
I was hoping it will work automatically 🤞
|
Seems like this bit needs some polyfilling or such 🤔 |
|
Release of other tool going up right now 👍 |
Ocramius
left a comment
Ocramius
left a comment
There was a problem hiding this comment.
I think we should tune this back a notch: we can move tools that don't share address space, but not tools that do share it 🤔
Let's see how this goes first, but caution is advised.
Happy to include sources as a "dependcency" from the tools directory though: that approach is smart too 👍
| "phpstan/phpstan-phpunit": "^1.3.16", | ||
| "phpunit/phpunit": "^10.5.12", | ||
| "vimeo/psalm": "5.23.0", | ||
| "roave/infection-static-analysis-plugin": "^1.34.0" |
There was a problem hiding this comment.
This tool shares the same address space as the main library: moving it here may be a mistake (same for phpunit).
I think we should only move tools that operate completely independently, which currently includes phpcs, phpstan (not psalm, due to the coupling with the test suite mutations).
That said, I'm fine with trying this out anyway 👍
| "Roave\\BetterReflection\\": "src" | ||
| } | ||
| }, | ||
| "autoload-dev": { |
There was a problem hiding this comment.
This will also mess with editors 🤔
I'm not convinced of moving everything, but I'm open to trying it, for now 🤔
| "php": "8.1.99" | ||
| } | ||
| }, | ||
| "autoload": { |
There was a problem hiding this comment.
Thinking of this further, we should probably add the parent package as a dependency, not as an autoload definition: that makes the parent package able to declare its own dependencies, and the tooling follows only
|
I’m sorry - there was “approved” so I merged it. |
|
That's fine: it doesn't currently affect downstream, and we can always go back: I just thought this was splitting out a couple tools, but I misread from my phone, so I was too quick 😬 Let's see how this works out meanwhile 👍 |
|
Just another idea - would it be possible to prepare phar for https://github.com/Roave/infection-static-analysis-plugin ? Dependencies will be packed in the Phar |
|
IMO no:
Also, the tool sharing memory space with the program is a problem, when used from a phar: that's why I still firmly believe that using tools like phpunit from a phar is a mistake. |
|
BTW, it would be fine if address space wasn't shared (most static analysis tools), but not for this |
|
Hmm, it looks the PR was not successful. We are still not able to upgrade PhpParser: #1387 PhpParser is used in tests so if we upgrade it we will have problem with Psalm and Infection dependencies again :( |
2 participants
No description provided.