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

Another attempt to add Compiler + upgrade to PHPStan 0.12 #2373

Merged
merged 12 commits into from
Dec 9, 2019

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Nov 30, 2019

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Nov 30, 2019

Fuckups

Symfony not phar friend for excluded in PSR-4 autodiscovery

 services:
     Rector\TypeDeclaration\:
         resource: '../src'
         exclude:
-         	- '../src/ValueObject/SpecificFile.php'
+         	- '../src/ValueObject/*'

✔️

Symfony autodsicovery hates slash

 services:
     Rector\TypeDeclaration\:
-        resource: '../src/'
+        resource: '../src'

✔️

SHA1 cannot be verified

  • Error: atal error: Uncaught PharException: phar "/home/travis/build/rectorphp/rector/compiler/build/box.phar" SHA1 signature could not be verified: broken signature in /home/travis/build/rectorphp/rector/compiler/build/box.phar on line 14
  • Possible bug: lf/crlf line endings on push to githbu
  • Solution?

Solution remove this from .gitattributes:

-# Set default behavior, in case users don't have core.autocrlf set.
-* text=auto
-* text eol=lf

✔️

Box is not compatible with multipes bins

Before:

  • bin/rector
  • bin/autoload.php
  • bin/container.php

Why? To separate logic to standalone functions by file.

Now:

  • bin/rector

With strict typed classes written on the bottom of file and used above.

✔️

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Nov 30, 2019

~~Fuckup post is comming :D ~~

Done: How to Box Symfony App to PHAR without Killing Yourself

@gnutix
Copy link
Contributor

gnutix commented Dec 5, 2019

Hey @TomasVotruba ! Just discovered this PR, awesome work! :D Is it already testable somehow ? I've cd ~/dev/oss/rector && git fetch && git checkout compiler && cd compiler && bin/compile, which gave me tmp/rector.phar.

Now when I cd into our project and run php ~/dev/oss/rector/tmp/rector.phar, it gives :

PHP Fatal error:  Uncaught _HumbugBox24440d0b0be5\Nette\FileNotFoundException: File '/composer/vendor/phpstan/phpstan-symfony/extension.neon' is missing or is not readable. in phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/di/src/DI/Config/Loader.php:29
Stack trace:
#0 phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/di/src/DI/Config/Loader.php(43): _HumbugBox24440d0b0be5\Nette\DI\Config\Loader->load('/composer/vendo...', false)
#1 phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/di/src/DI/Compiler.php(98): _HumbugBox24440d0b0be5\Nette\DI\Config\Loader->load('/home/gnutix/de...', false)
#2 phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/bootstrap/src/Bootstrap/Configurator.php(186): _HumbugBox24440d0b0be5\Nette\DI\Compiler->loadConfig('/home/gnutix/de...', Object(_HumbugBox24440d0b0be5\Nette\DI\Config\Loader))
#3 phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/di/src/DI/ContainerLoader.php(101): _HumbugBox24440d0b0be5\Nette\Configurator->generateCon in phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/di/src/DI/Config/Loader.php on line 29

Fatal error: Uncaught _HumbugBox24440d0b0be5\Nette\FileNotFoundException: File '/composer/vendor/phpstan/phpstan-symfony/extension.neon' is missing or is not readable. in phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/di/src/DI/Config/Loader.php:29
Stack trace:
#0 phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/di/src/DI/Config/Loader.php(43): _HumbugBox24440d0b0be5\Nette\DI\Config\Loader->load('/composer/vendo...', false)
#1 phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/di/src/DI/Compiler.php(98): _HumbugBox24440d0b0be5\Nette\DI\Config\Loader->load('/home/gnutix/de...', false)
#2 phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/bootstrap/src/Bootstrap/Configurator.php(186): _HumbugBox24440d0b0be5\Nette\DI\Compiler->loadConfig('/home/gnutix/de...', Object(_HumbugBox24440d0b0be5\Nette\DI\Config\Loader))
#3 phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/di/src/DI/ContainerLoader.php(101): _HumbugBox24440d0b0be5\Nette\Configurator->generateCon in phar:///home/gnutix/dev/oss/rector/tmp/rector.phar/vendor/nette/di/src/DI/Config/Loader.php on line 29

We use PHPStan (0.11, not updated to 0.12 yet) inside a Docker container (because of PHP 7.2 clash), and we load extensions with "internal" paths (in the container) like so :

# phpstan.neon
includes:
    # These paths are within the Docker container
    - '/composer/vendor/phpstan/phpstan-symfony/extension.neon'
    - '/composer/vendor/phpstan/phpstan-phpunit/extension.neon'
    - '/composer/vendor/phpstan/phpstan-doctrine/extension.neon'
    - '/composer/vendor/phpstan/phpstan-webmozart-assert/extension.neon'
    - '/composer/vendor/jangregor/phpstan-prophecy/src/extension.neon'

I guess that's the culprit ? I can't use PHPStan 0.12 as a phar, because installation requires "nikic/php-parser": "^4.3.0" which conflicts with my project requiring "ocramius/generated-hydrator": "^2.2" (as ^3.0 only supports PHP 7.3), itself only supporting nikic/php-parser: ~2.0|~3.0. So am I stuck until I can upgrade our project to PHP 7.3+ ?

@TomasVotruba
Copy link
Member Author

Check Travis job, there is steps to test it.
I know it fails on autoloader, but your issue should be fixed

@gnutix
Copy link
Contributor

gnutix commented Dec 5, 2019

I'm not really sure what you mean I'm supposed to look at inside .travis.yml : it contains tons of stuff, I'm a bit lost. What do you mean by "your issue should be fixed" ? (I'm using the latest compiler branch version... is there another, more up-to-date ? This branch hasn't been merged yet, right ?)

@escopecz
Copy link
Contributor

escopecz commented Dec 5, 2019

@gnutix check these lines: https://github.com/rectorphp/rector/pull/2373/files#diff-354f30a63fb0907d4ad57269548329e3R39-R49

To sum them up, the phar is being built automatically and the result is in the https://github.com/rectorphp/rector-prefixed repo. However, there are still issues with the final phar as Tomas noted.

@gnutix
Copy link
Contributor

gnutix commented Dec 5, 2019

Thanks @escopecz. I've downloaded the .phar from rectorphp/rector-prefixed, same error because of PHPStan's extension within the Docker image. Should I create a separate issue about this ?

@escopecz
Copy link
Contributor

escopecz commented Dec 5, 2019

That's odd. I'm getting ".phar://rector.phar/vendor/autoload.php", "phar://rector.phar/../../../vendor/autoload.phpHave you ran "composer update"? when I run that. Anyway, I don't think it makes sense to create new issues about features that haven't been merged to the core yet. Let's just help Tomas to get it working.

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Dec 5, 2019

@gnutix These lines are used to build phar:

https://github.com/rectorphp/rector/pull/2373/files#diff-354f30a63fb0907d4ad57269548329e3R27-R31

Any help with fixing/testing is appreciated

@TomasVotruba TomasVotruba force-pushed the compiler branch 3 times, most recently from 278ca13 to 5463ca8 Compare December 8, 2019 13:04
@TomasVotruba TomasVotruba force-pushed the compiler branch 3 times, most recently from 0ebcd9e to 55d8de0 Compare December 8, 2019 17:12
@TomasVotruba TomasVotruba changed the title Another attempt to add Compiler Another attempt to add Compiler + upgrade to PHPStan 0.12 Dec 8, 2019
@TomasVotruba
Copy link
Member Author

TomasVotruba commented Dec 9, 2019

Still not 100 % stable, but good enought to 🚢 it and test it in the wild.
Let's roll 👍

Thank you all for feedback and testing so far, much appreciated

@mpdude
Copy link
Contributor

mpdude commented Feb 18, 2021

This may be a stupid question... but where can I find the off-the-shelf .phar version?

There is no hint in the README, neither here nor over at https://github.com/rectorphp/rector-prefixed, they don't even contain .phar. I also checked the GitHub "Releases" in both repos and found nothing.

Since the .phar build (at least, in this PR) was set up for Travis, and since the project seems to use GHA now... maybe it was just lost?

@mpdude
Copy link
Contributor

mpdude commented Feb 18, 2021

Ah, the answer lies in e7f1963...

@TomasVotruba do you recall why you stopped building the .phar version?

IMO, having a .phar is the most simple and elegant solution for using a tool like Rector, since you have no interference with your project's dependencies, just grab a file and you're done.

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Feb 18, 2021

That's a valid question 👍

Here is the release Rector 0.9 info about moving from PHAR to scoped code:
https://getrector.org/blog/2020/12/28/rector-09-released

It's important stepping stone to downgradable releases, that are easy to use and debug.

That way it will be possible to do something like this:

# php 5.6
composer require symfony/console # 2.8 is installed
composer require rector/rector # 0.9 on PHP 5.6 is installed

The oldest PHP version code-bases are the biggest hell to upgrade, so our main goal is to help them out

@mpdude
Copy link
Contributor

mpdude commented Feb 18, 2021

Debugging phar-specific issues (guess you ment that?) for sure isn’t fun. Symfony (Console?) cannot be packed into a phar – I’ll take your word for it.

I guess it boils down to the same questions as in #5593:

Is there a requirement that the (major?) version of PHP that runs Rector must match the version of PHP currently required by the project?

Is this by design/unavoidable, or just a current bug?

Can/does/should Rector treat project code as „data“ only, or does it have to parse/load/interpret it at the runtime interpreter/language level?

I am so persistent on these particular questions because it seems to me that has great impact on how much sense it makes to invest into Docker images: In a container, at best we can keep Rector and all its dependencies fully isolated from the project (after all, that’s what it is all about). The PHP version, available extensions etc. when running Rector will almost never be in line with what the project itself uses at runtime.

That‘s all not an issue if the container runs something that will happily parse PHP 5 up to PHP 8 code as input.

But if the version of Rector (and at least also PHPStan, even in the „scoped“ variant) must match project dependencies, it‘s going to be hard to make that work with Docker images... 👨🏻‍💻

Am I making sense?

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Feb 18, 2021

Symfony can be packed into phar, but it's a road to hell: https://tomasvotruba.com/blog/2019/12/02/how-to-box-symfony-app-to-phar-without-killing-yourself/ + Also,realpath() in phar does not work.

As for project vs Rector dependencies, they can differ: https://getrector.org/blog/2020/01/20/how-to-install-rector-despite-composer-conflicts

@mpdude
Copy link
Contributor

mpdude commented Feb 19, 2021

Sorry, I don't understand it...

You removed the PHAR:

Here is the release Rector 0.9 info about moving from PHAR to scoped code: https://getrector.org/blog/2020/12/28/rector-09-released

... suggesting to use the scoped version and to install it using Composer, so that Composer can find versions compatible with the project's other requirements?

And then, when I asked why/how/which dependencies and/or language versions need to be in sync between the project and Rector, you answer

they can differ: https://getrector.org/blog/2020/01/20/how-to-install-rector-despite-composer-conflicts

... pointing me to a blog post that, at the bottom, announces the .phar version. I am confused 😕.

@TomasVotruba
Copy link
Member Author

... suggesting to use the scoped version and to install it using Composer, so that Composer can find versions compatible with the project's other requirements?

Yes. Use the rector-scoped

The second post is about phar, but that's because it's older. Take it as synonymum for rector-scoped

@glensc
Copy link

glensc commented Mar 26, 2021

The third point in the blog:

it should start with the fact that you are committing the .phar to the repo, otherwise, it's confusing that you have the "spoiler" at the end of the paragraph, accidentally even mentioning a commit. 'we shipped' requires to have previous knowledge of your project and how you do the shipping.

@TomasVotruba
Copy link
Member Author

TomasVotruba added a commit that referenced this pull request May 27, 2022
rectorphp/rector-src@03da987 Try fix stub ReflectionUnionType not found in CI php 7.2 (#2373)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doesn't work with phpstan 0.12
5 participants