Skip to content
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

Extract build tools using composer-bin plugin #746

Closed
wants to merge 1 commit into from

Conversation

core23
Copy link
Member

@core23 core23 commented Oct 16, 2021

Subject

Try to fix the build.

This will increase the build time as two composer.json projects will be installed with every build job.

Refs: sonata-project/dev-kit#372

@@ -11,3 +11,5 @@ phpunit.xml
phpstan.neon
/.phpunit.result.cache
/docs/_build/
!/vendor-bin/tools/composer.lock
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using composer-bin, we should manage all vendor-bin files (including the LOCK file) via the dev-kit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean dispatching vendor-bin/tools/composer.json to all the others projects ?
Might be interesting, but then we'll install some useless phpstan extension like phpstan/phpstan-doctrine on non doctrine related projects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this direction (which gives me mixed feelings) this file should be dispatched, otherwise we need to maintain 2 composer json on each project which is a not good idea imho.

What I like about it is that we can handle all the tooling from dev kit and does not depend on each project having this correctly configured.

What I dont like is that we add more dependencies on this composer bin tool, when this can also be fixed by just installing phpunit on the project composer.json directly (in fact phpunit is a direct dev dependency used a lot in tests folder, which is not the same case for phpstan for example)

Not sure about the composer.lock either, It could be a good idea if we have some dependabot upgrading this file on dev-kit or a bad idea if we have to manually do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact phpunit is a direct dev dependency used a lot in tests folder

And symfony decided to promote requiring both phpunit and phpunit bridge for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can "build" the composer.json file in dev kit per project with the needed extensions/libs etc. and dispatch it

Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting solution

I don't think the lock file should be committed. It would require us to bump regularly the tool version (especially phpstan/psalm) and fix new errors for every projects. This would require a lot of extra work.
Without commiting the lock files, it will be bumped at every test run and we just have to fix when a new error happen. It happens some times, never blocked us and we avoided to do something every other times the dependency got a new version.

"description": "Development tools that do not conflict the project dependencies",
"require-dev": {
"friendsofphp/php-cs-fixer": "^3.0",
"phpspec/prophecy-phpunit": "^2.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We got rid of prophecy and prefer to use mock. We should not add it again IMHO.

"phpstan/phpstan": "^0.12.84",
"phpstan/phpstan-phpunit": "^0.12.16",
"phpstan/phpstan-symfony": "^0.12.21",
"phpunit/phpunit": "^8.5 || ^9.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to keep ^8 support ?

"type": "project",
"description": "Development tools that do not conflict the project dependencies",
"require-dev": {
"friendsofphp/php-cs-fixer": "^3.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good things since it will allow to use this in the github actions

@@ -91,10 +84,10 @@
},
"scripts": {
"post-install-cmd": [
"[ $COMPOSER_DEV_MODE -eq 0 ] || vendor/bin/simple-phpunit install"
"@composer bin all install --ansi"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ $COMPOSER_DEV_MODE -eq 0 ] ||

Was allowing to only run this in dev mode ; does bin composer do the same ?

@@ -11,3 +11,5 @@ phpunit.xml
phpstan.neon
/.phpunit.result.cache
/docs/_build/
!/vendor-bin/tools/composer.lock
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean dispatching vendor-bin/tools/composer.json to all the others projects ?
Might be interesting, but then we'll install some useless phpstan extension like phpstan/phpstan-doctrine on non doctrine related projects.

@jordisala1991
Copy link
Member

This solution is not needed anymore (for now).

@core23 core23 deleted the tools branch October 18, 2021 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants