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

Cache doesn't work when the composer.lock is in a sub directory. #4941

Closed
VincentLanglet opened this issue Jan 7, 2021 · 20 comments · Fixed by #9970
Closed

Cache doesn't work when the composer.lock is in a sub directory. #4941

VincentLanglet opened this issue Jan 7, 2021 · 20 comments · Fixed by #9970
Labels

Comments

@VincentLanglet
Copy link
Contributor

When I run

symfony/vendor/bin/psalm --error-level=8

I get the output:

Checks took 138.87 seconds and used 1,431.678MB of memory

If I run again the command, the output is

Checks took 140.04 seconds and used 1,431.678MB of memory

I tried --no-cache, I got the output

Checks took 89.93 seconds and used 758.369MB of memory

I also tried, --no-diff, I got the output

Checks took 121.00 seconds and used 1,431.675MB of memory

This is my psalm config

<psalm
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xmlns="https://getpsalm.org/schema/config"
    xsi:schemaLocation="https://getpsalm.org/schema/config symfony/vendor/vimeo/psalm/config.xsd"
    allowPhpStormGenerics="true"
    autoloader="symfony/bin/.phpunit/phpunit/vendor/autoload.php"
    cacheDirectory="cache/psalm"
    errorLevel="8"
    findUnusedPsalmSuppress="true"
    reportInfo="false"
>
    <projectFiles>
        <directory name="symfony/src"/>
        <directory name="symfony/tests"/>
        <ignoreFiles>
            <file name="symfony/src/Repository/.phpstorm.meta.php"/>
            <file name="symfony/tests/.phpstorm.meta.php"/>
            <directory name="symfony/src/Migrations"/>
            <directory name="symfony/vendor"/>
        </ignoreFiles>
    </projectFiles>
    <plugins>
        <pluginClass class="Psalm\PhpUnitPlugin\Plugin"/>
        <pluginClass class="Psalm\SymfonyPsalmPlugin\Plugin">
            <containerXml>symfony/var/cache/dev/srcAssuranceVie_KernelDevDebugContainer.xml</containerXml>
        </pluginClass>
    </plugins>
</psalm>
@psalm-github-bot
Copy link

Hey @VincentLanglet, can you reproduce the issue on https://psalm.dev ?

@VincentLanglet
Copy link
Contributor Author

I tried another cacheDirectory, and I takes 90s now. Maybe it was because the cache directory was previously in the shared folders of my vagrant.

But I don't understand why running psalm with or without the cache takes the same time. What's the interest of the cache then ?

@orklah
Copy link
Collaborator

orklah commented Jan 7, 2021

This is generally not the case. Maybe there's something on your codebase that makes psalm write too much in cache to a point where reading the cache is detrimental?

On my work codebase, Psalm will run out of memory(12Go) just trying to write the cache, not to talk about reading it.

Maybe a blackfire profile could tell us more if you want to investigate

EDIT: you could also try to simplify your config file to see if you can replicate without exotic config. Try only analyzing src/ without plugins and without your custome autoloader

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Jan 7, 2021

EDIT: you could also try to simplify your config file to see if you can replicate without exotic config. Try only analyzing src/ without plugins and without your custome autoloader

With this config:

<psalm
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xmlns="https://getpsalm.org/schema/config"
    xsi:schemaLocation="https://getpsalm.org/schema/config symfony/vendor/vimeo/psalm/config.xsd"
    errorLevel="8"
    findUnusedPsalmSuppress="true"
    reportInfo="false"
>
    <projectFiles>
        <directory name="symfony/src"/>
    </projectFiles>
</psalm>

Running psalm give this output:

Checks took 64.62 seconds and used 740.216MB of memory

If I run it again, it takes the same time.

Without cache:

Checks took 67.89 seconds and used 333.828MB of memory

With phpstan the cache is working fine, so I don't know what would cause an issue in my codebase.

@muglug
Copy link
Collaborator

muglug commented Jan 7, 2021

what happens if you run it a third time?

@VincentLanglet
Copy link
Contributor Author

what happens if you run it a third time?

First run

Checks took 104.64 seconds and used 1,432.954MB of memory

Second run

Checks took 107.61 seconds and used 1,432.954MB of memory

Third run

Checks took 89.06 seconds and used 1,432.954MB of memory

Fourth run

Checks took 87.32 seconds and used 1,432.954MB of memory

So it seems to be more impactful after two runs indeed. Why ?

But still, with --no-cache, I get

Checks took 87.75 seconds and used 759.129MB of memory

So I have no benefit.

@weirdan
Copy link
Collaborator

weirdan commented Jan 7, 2021

When cache is in effect you should get no progress bar and No files analyzed message on the second or third run (assuming there's no changes to the codebase between runs). If that doesn't happen then cache is not working.

Mind running with --debug and posting the output in a gist?

@VincentLanglet
Copy link
Contributor Author

When cache is in effect you should get no progress bar and No files analyzed message on the second or third run (assuming there's no changes to the codebase between runs). If that doesn't happen then cache is not working.

It never happen ! So cache is not working (but still using more memory than no-cache).

Mind running with --debug and posting the output in a gist?

I got

'Composer lockfile change detected, clearing cache'

Which I found weird ; so I looked at the vendor.

The ProjectCacheProvider::hasLockfileChanged()' is returning true` because the composer lock is not found.

public function hasLockfileChanged() : bool
    {
        if (!file_exists($this->composer_lock_location)) {
            return true;
        }

I think it's because the composer.lock is not in the root file of the project, but in my symfony/ folder.
I made a try

ln -s symfony/composer.lock composer.lock

And now cache is working.

When running composer, we're using --working-dir.
Is there an option for psalm ? To tell where is the composer.lock ?

I can try to add a such option if you want, but I will need some help

@orklah
Copy link
Collaborator

orklah commented Jan 7, 2021

oh nice catch! I wonder if Composer provide a way to find the lock file automatically

@weirdan
Copy link
Collaborator

weirdan commented Jan 7, 2021

@VincentLanglet Do you also have to specify composer autoloader manually in psalm.xml, e.g. autoloader="symfony/vendor/autoload.php"?

@weirdan
Copy link
Collaborator

weirdan commented Jan 7, 2021

I wonder if Composer provide a way to find the lock file automatically

Unlikely, there wouldn't be a need to specify it as CLI argument if that was the case.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Jan 7, 2021

The psalm code use

$file_name = getenv('COMPOSER') ?: 'composer.json';

here: https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Composer.php#L24

So I thought that

COMPOSER='symfony/composer.json' symfony/vendor/bin/psalm

could work but the line

$file_name = basename(trim($file_name));

is removing the symfony.

I tried to comment it temporary and then I get Could not find any composer autoloaders in ....

@VincentLanglet Do you also have to specify composer autoloader manually in psalm.xml, e.g. autoloader="symfony/vendor/autoload.php"?

I use

autoloader="symfony/bin/.phpunit/phpunit/vendor/autoload.php"

because I'm using the symfony phpunit bridge.

But I tried without and I don't need to specify the autoloader file to run psalm. (phpunit will just not be found)
So it seems that psalm is finding correctly the file autoload.php in symfony/vendor even if it looks for the composer.lock in the root directory @weirdan

@weirdan
Copy link
Collaborator

weirdan commented Jan 7, 2021

So I thought that COMPOSER='symfony/composer.json' symfony/vendor/bin/psalm could work

COMPOSER env var sets the name of the composer.json/composer.lock file (https://getcomposer.org/doc/03-cli.md#composer)

But I tried without and I don't need to specify the autoloader file to run psalm. (phpunit will just not be found)

Interesting. Can you trace how Psalm is finding your vendor/autoload.php? It should happen in src/command_functions.php, the function is called requireAutoloaders(). Chances are Composer::getJsonFilePath() is called with an argument different to what we use with Composer::getLockFilePath().

@VincentLanglet
Copy link
Contributor Author

But I tried without and I don't need to specify the autoloader file to run psalm. (phpunit will just not be found)

Interesting. Can you trace how Psalm is finding your vendor/autoload.php? It should happen in src/command_functions.php, the function is called requireAutoloaders(). Chances are Composer::getJsonFilePath() is called with an argument different to what we use with Composer::getLockFilePath().

Continuing the debugging:

This is a dump of the autoload_roots: https://github.com/vimeo/psalm/blob/master/src/command_functions.php#L63

array(2) {
  [0]=>
  string(42) "/var/www/vagrant.assurancevie.com/current/"
  [1]=>
  string(68) "/var/www/vagrant.assurancevie.com/current/symfony/vendor/vimeo/psalm"
}

With the first path,

  • No autoloader is found
  • No file composer.json is found
    So it does nothing, and then with the second path
  • A $nested_autoload_file is found

So I thought that COMPOSER='symfony/composer.json' symfony/vendor/bin/psalm could work

COMPOSER env var sets the name of the composer.json/composer.lock file (https://getcomposer.org/doc/03-cli.md#composer)

If you look at https://github.com/composer/composer/blob/5df1797d20c6ab1eb606dc0f0d76a16ba57ddb7f/src/Composer/Factory.php#L233 Composer is not using basename() like it's done in psalm https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Composer.php#L25.

I just tried and composer does support a path like symfony/composer.json.
I think it's better to remove this, this would allows support for COMPOSER='symfony/composer.json'

But then there is still something to do, because with this line commented and COMPOSER='symfony/composer.json'
With the first path,

  • No autoloader is found
  • A file composer.json is found

And I get an error.

Could not find any composer autoloaders in /var/www/vagrant.assurancevie.com/current/
Add a --root=[your/project/directory] flag to specify a particular project to run Psalm on.

So the working command is (with the comment of basename)

COMPOSER='symfony/composer.json' symfony/vendor/bin/psalm --root=symfony/

I think that

symfony/vendor/bin/psalm --root=symfony/

should be enough. So it required to fix the $currentDir passed to getJsonFilePath and getLockFilePath.

I looked at
https://github.com/vimeo/psalm/search?q=getJsonFilePath
https://github.com/vimeo/psalm/search?q=getLockFilePath
and I found three weird things.

1. My issue

It seems like my issue is here:

$current_dir = $config->base_dir;

The --root argument is kept only if I don't resolve from config.
So symfony/vendor/bin/psalm --root=symfony/ works with resolveFromConfigFile="false".

But it's weird because, if I look at the doc

If this is enabled, relative directories mentioned in the config file will be resolved relative to the location of the config file. If it is disabled, or absent they will be resolved relative to the working directory of the Psalm process.
There is no mention about the composer.json.

2. The php version

getPHPVersionFromComposerJson is using the base_dir to check the php version https://github.com/vimeo/psalm/blob/master/src/Psalm/Config.php#L2066
And this base_dir can be totally different from the one where we look for the autoload.php file.

In my case, if I run symfony/vendor/bin/psalm, even if the requireAutoloaders is finding the autoload.php file in symfony/vendor/ (because I run symfony/vendor/bin/psalm and it's a nested autoload), it keep trying to find composer.json in the root for the php version.

3. The plugin list

Another weird thing is that psalm-plugin.php does not care about the root argument

requireAutoloaders($current_dir, false, $vendor_dir);

By the way, I found another issue during my tests: #4952

@VincentLanglet
Copy link
Contributor Author

Moving the psalm.xml in my symfony directory and running symfony/vendor/bin/psalm --config=symfony/psalm.xml is a workaround.

But I think my previous message is interesting and may lead to some psalm improvement @weirdan @muglug. WDYT ?

@weirdan weirdan added the bug label Jan 7, 2021
@weirdan
Copy link
Collaborator

weirdan commented Jan 7, 2021

Thanks for the detailed write up. Mind sharing your project directory structure? This would help to reproduce it locally.

@VincentLanglet VincentLanglet changed the title Psalm is worst with cache than without cache Cache doesn't work when the composer.lock is in a sub directory. Jan 7, 2021
@VincentLanglet
Copy link
Contributor Author

The project directory is

root
 |- folder1
 |- folder2
 |- symfony
 |    |- folder3
 |    |- folder4
 |    |- src
 |    |- tests
 |    |- vendor
 |    |- composer.json
 |    |- composer.lock
 |
 |- psalm.xml

I run symfony/vendor/bin/psalm in the root directory.

@weirdan
Copy link
Collaborator

weirdan commented Jan 8, 2021

Do you also check files outside symfony folder? I think the easiest workaround (that you also figured out) given the config you posted and your directory structure would be to move psalm.xml to symfony folder and just run Psalm from there.

@VincentLanglet
Copy link
Contributor Author

Do you also check files outside symfony folder?

No

I think the easiest workaround (that you also figured out) given the config you posted and your directory structure would be to move psalm.xml to symfony folder and just run Psalm from there.

Yeah sure, it was just easier to have the config in the root dir.

And the current behavior is misleading.

  • Either it should be fully working (with the cache)
  • Either I should have an error message from the beginning.
    Without this thread, I would have never had the benefit of the cache.

Currently, the psalm file is

Maybe it should be done in the opposite way

  • Building the config file
  • Looking for autoload.php, using the config->baseline as the currentDir.

This way, the composer.json used for the autoload.php is the same file that the one use for the php version, or the cache.

@mitelg
Copy link
Contributor

mitelg commented Feb 12, 2021

We are facing the same issue, that the cache is not used, but we have slightly different project setting

root
 |- tools
 |    |- vendor
 |    |- composer.json
 |    |- composer.lock
 |- public
 |- project
 |    |- src
 |    |- psalm.xml
 |    |- composer.json
 |
 |- composer.json
 |- composer.lock

We have two different templates. one for developing "project" and one for running "project" production. These two templates have basically just one requirement which is vendor-name/project. in the dev setup, we are using a path repository in the root composer.json

    "repositories": [
        {
            "type": "path",
            "url": "project",
            "options": {
                "symlink": true
            }
        }
    ],

In the production template this normally required and located in vendor.

So in our dev template, we are using psalm, which is installed within the tools directory.
We are running it from root with the config located in project to check the code located in project/src.

The missing composer.lock hint, did let me try something: I copied the composer.lock from root into project and it now works.

I'm also pretty sure, that the cache worked properly in some 3.x release before. I hope this helps to find a solution 😊

//Edit: Just tested 3.9.0. With this, the caching works as expected. Same with 3.15.0
Also interesting that the check with 3.9.0 tooks 74.18 seconds and 2,653.057MB of memory
and with 3.15.0 it was already 190.08 seconds and 2,683.117MB of memory
the last 3.18.2 release is also ok. seems the issue was introduced somewhere in the 4.x releases.
(We are currently trying to update from 3.18.2 to current 4.5.0 version, because we just increased the minimum required PHP version to 7.4)

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 a pull request may close this issue.

5 participants