-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AppRepository to enable tools inside apps #9792
base: main
Are you sure you want to change the base?
Conversation
f33aae6
to
8303ea3
Compare
fb860f7
to
9542c16
Compare
shouldn't that be |
It would be nice if composer scripts can find |
I think adding a separate composer.json file for this which may exist in different directories based on the project is really a step backward from standardized composer.json at the root of the project. If this is to be implemented I think it should be as part of the project's composer.json as discussed in previous issues and the one you linked. |
@naderman I totally agree: as I suggested in the description, this PR is a step towards this target. I feel safer to take this path than to dream about an issue without anyone actually contributing some code :) |
If I for example installed phpstan and phpunit as a tools, would I be able to analyse phpunit tests with phpstan? From what I understand, phpstan would know nothing about phpunit. I think phars are better solution for tools. Phar can be published as a separate package, with no dependencies, so it can be still installed via Composer. For example phpstan do that - phpstan/phpstan-src is published via phpstan/phpstan phar |
@mabar yes, phpstan would read |
@gmsantos this works already, what else would you need?
|
@nicolas-grekas it would be related with this note from the docs:
If you just use phpunit, ./tools/test/phpunit would not be found, i.e.
|
9542c16
to
7a7d23d
Compare
Nice experiment! How would the autoloaders interact in such a case? I can see multiple situations in which I would need to load the app and multiple tools at the same time... For example, PHPUnit, phpspec/prophecy-phpunit (and its transient dependency Prophecy) and the app's code. Plus, I would want to run static analysis (and its plugins!) on top of that. |
@Jean85 there is one autoloader per "tools" directory, which can load the app or not depending on the |
@nicolas-grekas and what about transient dependencies? Let's say, both the app and the tool require symfony/console (which is very likely), the first to be loaded wins? Wouldn't that create clashing? |
I suppose you mean transitive dependencies, right? What I wrote in the description: the |
So, let me rephrase is to see if I get this right: transitive (yep, sorry I meant that) dependencies that are present in both the app and the tool wouldn't get duplicated, but instead the tool will use the same from the app, with a symlink if needed. You approach will reduce the "clash" because transitive dependencies that do NOT overlap with the app will be resolved in a different tree, reducing the "pollution" that the app would get from the normal Did I get this right? If this is the case, we still have the (big) issue of common transitive deps, and that it's a tough problem to solve. |
I think you've got this right. Which issue are you talking about? To me they are all solved:
|
I'm just pondering the full impact of your solution... surely you solved a big chunk of possible conflicts, but the tools still needs to be compatible with your own set of dependencies. Anyway, as said above, this shouldn't be a big issue for applications (where lock is present and target is known) and for libraries like Symfony too (where you should aim to reduce your dependencies). Good! 👍 |
I see that this looks like a valid workaround for the problem but is it the right way to get it solved? Aren't we trying to abuse Composer? Can we collect and compare how others are dealing with this problem in the industry? (Like how it happened with Enum PSR recently.) I am more like with those who asked would not be better using PHAR for development dependencies. Would not it be better figuring out best practices around that instead of trying to add tricks to a dependency manager? PHAR is similar to Snap/Flatpak in my vocabulary and they were born for a reason. Related: https://tomasvotruba.com/blog/2019/12/02/how-to-box-symfony-app-to-phar-without-killing-yourself/ |
The limitation of the PHAR approach is that every single maintainer has to apply it to its project; if we can bake in a similar solution inside Composer, maybe with a scoper too, everyone will benefit from that for free. |
Is it still needed? |
This is a tentative PR towards a better separation of tools' dependencies vs app's dependencies. This is related to #9636 and the discussion that is happening there.
Inside a dummy app, I created a
tools/test/composer.json
with the following content:Running
composer install
inside thistools/test/
directory installs PHPUnit and its dependencies invendor/composer/tools/test
, with symlinks to the root app for shared dependencies.Since many apps don't have a
name
in theircomposer.json
file, theapp/app
that you see in therequire
section is a convention I added to allow tools to reference the app itself as a dependency when needed (for PHPUnit typically). If a tool doesn't need access to the app's source code, this entry should be just skipped.By default, packages found in the app's
vendor/
directory are locked: a tool that shares a dependency with an app will always resolve that dependency to the same exact version as the app. This can be changed by making the aboveapp
repository "non-canonical", as explained in Composer's doc.I feel like this PR is a small but useful step towards splitting tools' deps from app's ones. The DX could be improved for sure, but this can be done in future PRs I think.
Before adding tests/doc: is this worth pursuing? I think yes :) What about you?
TL;DR: with this PR, running phpunit is done by typing
$> ./vendor/bin/phpunit
and phpunit's deps don't mess up with your app's ones.