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

Running rector from Docker #2186

Closed
gnutix opened this issue Oct 21, 2019 · 26 comments · Fixed by #2221
Closed

Running rector from Docker #2186

gnutix opened this issue Oct 21, 2019 · 26 comments · Fixed by #2221
Assignees

Comments

@gnutix
Copy link
Contributor

gnutix commented Oct 21, 2019

I've recently changed my setup to run Rector from Docker instead of including it in my composer.json to avoid #1899 . And it feels to me like one of the "migration step" is not currently documented, which is updating the paths in my rector.yaml file's imports section, like this :

 imports:
-    - { resource: 'vendor/rector/rector/config/set/code-quality/code-quality.yaml' }
-    - { resource: 'vendor/rector/rector/config/set/coding-style/coding-style.yaml' }
-    - { resource: 'vendor/rector/rector/config/set/solid/solid.yaml' }
+    - { resource: '/rector/config/set/code-quality/code-quality.yaml' }
+    - { resource: '/rector/config/set/coding-style/coding-style.yaml' }
+    - { resource: '/rector/config/set/solid/solid.yaml' }
... etc ...

Or did I "do it wrong" ?

@JanMikes
Copy link
Contributor

Hi @gnutix please to help me better understand your situation, could you paste exact command you are trying to run?

@gnutix
Copy link
Contributor Author

gnutix commented Oct 21, 2019

So I've changed from running Rector using this command :

vendor/bin/rector --ansi process src/ tests/

to this one :

docker run -v $(pwd):/project rector/rector:0.5.17 process /project/src /project/tests --config /project/rector.yaml --autoload-file /project/vendor/autoload.php

Which required, in order to work, to change my rector.yaml as described above. And I've seen no such changes explained in the documentation anywhere, so I'm wondering if the project is simply missing some documentation or if that change is "unexpected" and shouldn't be necessary ? (and therefore it might be caused by a bug)

@JanMikes
Copy link
Contributor

I am looking into that.

Expected and desired behaviour is definitely to work with Rector in docker in same way, without need of changing paths in your config.

@JanMikes
Copy link
Contributor

Anyways i have an idea - did your "migration" to docker include uninstalling rector from your composer dependencies? That would lead into vendor/rector/rector/config/set/code-quality/code-quality.yaml file not existing in your application anymore, therefore i would understand why you had to make such change.

@gnutix
Copy link
Contributor Author

gnutix commented Oct 23, 2019

Indeed. Removing vendor/rector/rector was the point, as I wanted to get rid of the multiple definition exists for class %s phpstorm inspection caused by the stubs classes.

@JanMikes
Copy link
Contributor

@TomasVotruba i would like your opinion here.

This is caused by file vendor/rector/rector/config/set/code-quality/code-quality.yaml obviously no longer exists, when rector/rector removed from dependencies.

We can either:

  1. Add info into readme to docker chapter that this change might be required
  2. Add some env var to docker, etc RECTOR_SOURCE_DIR, then in TolerantRectorYamlFileLoader override parseImports, check via getenv('RECTOR_SOURCE_DIR') and if it exists, then replace vendor/rector/rector with that value. This is kind of magic but would solve this issue (at this moment i can not come with another usage than this very specific use case)
  3. Just close this issue as it is solved and not relevant to rector/docker

Thank you in advance for your feedback

@TomasVotruba
Copy link
Member

I don't understand the issue. How other container PHP apps require their configs?

@JanMikes
Copy link
Contributor

Okay, i will try to explain again and more verbose 😄

This is not related to Docker at all.

Composer.json:

"require-dev": {
        "rector/rector": "dev-master"
    },

Now vendor/rector/rector/config/set/code-quality/code-quality.yaml exists so you can use it in import.

Then rector/rector dependency is removed from composer.json and vendor/rector/rector/config/set/code-quality/code-quality.yaml no longer exists.

But, if you run rector from docker, this file is available at /rector/config/set/code-quality/code-quality.yaml and therefore you can import it in your config.

I proposed 3 solutions. Can we pick one or come with another?

@JanMikes JanMikes removed the docker label Oct 25, 2019
@gnutix
Copy link
Contributor Author

gnutix commented Oct 25, 2019

Would there be a way to allow relative paths from the rector/config folder or a parameter like %rector.config% ?

imports:
    - { resource: 'set/code-quality/code-quality.yaml' }
    - { resource: '%rector.config%/set/code-quality/code-quality.yaml' }

@TomasVotruba
Copy link
Member

TomasVotruba commented Oct 25, 2019

Where is this in our code?

- { resource: 'vendor/rector/rector/config/set/code-quality/code-quality.yaml' }

The path should be indeed relative

@gnutix
Copy link
Contributor Author

gnutix commented Oct 25, 2019

The path should be indeed relative

Yeah, but it's relative to the position of the rector.yaml file, which changes depending if you're running it "standard" or within the Docker container (as the Rector source code is not mounted in vendor/rector).

@TomasVotruba
Copy link
Member

TomasVotruba commented Oct 25, 2019

Of course, that's expected. If the config is missing, there should be an error

@TomasVotruba
Copy link
Member

How does PHPStan approach this?

@JanMikes
Copy link
Contributor

How does PHPStan approach this?

I tested it and PHPStan has %rootDir% variable which resolves into directory where phpstan is installed. It is very close to what @gnutix proposed.

@TomasVotruba
Copy link
Member

I'm open to PR for the variable then

@TomasVotruba
Copy link
Member

TomasVotruba commented Oct 30, 2019

This will help: #2221 :)

@gnutix
Copy link
Contributor Author

gnutix commented Nov 1, 2019

I've tried using this new syntax on latest / 0.5.19 / 0.5.18, but I got this error :

$ docker run -v $(pwd):/project rector/rector:latest process /project/src /project/tests --config /project/rector.yaml --autoload-file /project/vendor/autoload.php --ansi --dry-run

Fatal error: Uncaught Nette\FileNotFoundException: File '/rector/vendor/symplify/phpstan-extensions/config/config.neon' is missing or is not readable. in /rector/vendor/nette/di/src/DI/Config/Loader.php:43
Stack trace:
#0 /rector/vendor/nette/di/src/DI/Config/Loader.php(60): Nette\DI\Config\Loader->load('/rector/vendor/...', false)
#1 /rector/vendor/nette/di/src/DI/Compiler.php(127): Nette\DI\Config\Loader->load('/rector/phpstan...', false)
#2 /rector/vendor/nette/bootstrap/src/Bootstrap/Configurator.php(258): Nette\DI\Compiler->loadConfig('/rector/phpstan...', Object(Nette\DI\Config\Loader))
#3 /rector/vendor/nette/di/src/DI/ContainerLoader.php(119): Nette\Configurator->generateContainer(Object(Nette\DI\Compiler))
#4 /rector/vendor/nette/di/src/DI/ContainerLoader.php(79): Nette\DI\ContainerLoader->generate('Container_48b06...', Array)
#5 /rector/vendor/nette/di/src/DI/ContainerLoader.php(44): Nette\DI\ContainerLoader->loadFile('Container_48b06...', Array)
#6 /rector/vendor/nette/bootstrap/src/Bootstrap/Configurator.php(2 in /rector/vendor/nette/di/src/DI/Config/Loader.php on line 43

And on 0.5.20 / 0.5.17 and lower, I got :

$ docker run -v $(pwd):/project rector/rector:0.5.20 process /project/src /project/tests --config /project/rector.yaml --autoload-file /project/vendor/autoload.php --ansi --dry-run
Rector No version set (parsed as 1.0.0)@
Config file: ../project/rector.yaml

   1/822 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░]   0%

Fatal error: Declaration of PhpParser\Node\Name::getSubNodeNames() must be compatible with PhpParser\Node::getSubNodeNames(): array in /project/vendor/nikic/php-parser/lib/PhpParser/Node/Name.php on line 196

Locally, I'm running PHP 7.3, and the Docker image does too. Our project is using PHP 7.2. I find it weird that latest gives a different result than 0.5.20 (which is the latest release, right ?).

Here's my config : https://gist.github.com/gnutix/4080142ecce07696035e19bdcb8c3d31

@gnutix gnutix reopened this Nov 1, 2019
@TomasVotruba
Copy link
Member

TomasVotruba commented Nov 1, 2019

Always try the lastest version, if not dev-master. We develop rather quickly now :)

Import works correctly. The Fatal error is cause by issues related to: #177

@gnutix
Copy link
Contributor Author

gnutix commented Nov 5, 2019

I can only chose rector/rector:latest when running rector through Docker, right ?

Still gives me the Fatal error: Uncaught Nette\FileNotFoundException: File '/rector/vendor/symplify/phpstan-extensions/config/config.neon' is missing exception, so I can't use it via Docker...

@gnutix
Copy link
Contributor Author

gnutix commented Nov 6, 2019

@TomasVotruba I was indeed missing a call to docker pull rector/rector:latest (I would have thought that Docker updated the image if it used :latest, but nope), which fixed the "simplify/phpstan-extension" error. Doesn't fix the "Fatal error" on nikic/php-parser though :

$ docker pull rector/rector:latest && docker run --rm -v $(pwd):/project rector/rector:latest process -vvv --ansi --autoload-file /project/vendor/autoload.php --set phpunit60 /project/src

latest: Pulling from rector/rector
Digest: sha256:310293a15b9cd6fa740816609f6f398556c35d4c11c1f4f3310e6f68ecad242d
Status: Image is up to date for rector/rector:latest
docker.io/rector/rector:latest
Rector No version set (parsed as 1.0.0)@

[parsing] ../project/src/FirstClassBeingParsed.php

Fatal error: Declaration of PhpParser\Node\Name::getSubNodeNames() must be compatible with PhpParser\Node::getSubNodeNames(): array in /project/vendor/nikic/php-parser/lib/PhpParser/Node/Name.php on line 196

And #177 had not been touched for months, so I'm not sure it's really relevant ?

@gnutix gnutix reopened this Nov 6, 2019
@gnutix gnutix changed the title Running from Docker and rector.yaml imports Running rector from Docker Nov 6, 2019
@TomasVotruba
Copy link
Member

I have no idea how Docker works here. I don't use it with Rector

Until #177 is solved, you'll keep getting this error.
Please do not re-open issue anymore, as it's duplicated to #177

@gnutix
Copy link
Contributor Author

gnutix commented Nov 6, 2019

It seems to be more a duplicate of #2178

@TomasVotruba
Copy link
Member

Yes, there were many similar errors - just check "Fatal error" in Rector issues. But it has one common point of origin.

Here is the problem that happens:
https://3v4l.org/VT3kf

@gnutix
Copy link
Contributor Author

gnutix commented Nov 6, 2019

Indeed, my code is using ocramius/hydrated-generator, which depends on nikic/php-parser, and as we're running on PHP 7.2, it's restricted to a lower version which clashes with the version 4.x included in Rector's vendors.

So I looked deeper into this, and I've seen a comment of yours where you proposed to explain what would be needed to build Rector in a phar. Would you mind doing that ? I might then be able to give it a shot (as it would benefit the new project I'm working on to be able to run Rector).

@TomasVotruba
Copy link
Member

Let's continue in #177

@TomasVotruba
Copy link
Member

TomasVotruba added a commit that referenced this issue Apr 28, 2022
rectorphp/rector-src@8332d8c remove unused unwrapExpression(), use direct instanceof compare instead (#2186)
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 a pull request may close this issue.

3 participants