oxsecurity / megalinter

🦙 MegaLinter analyzes 50 languages, 22 formats, 21 tooling formats, excessive copy-pastes, spelling mistakes and security issues in your repository sources with a GitHub Action, other CI tools or locally.
https://megalinter.io
GNU Affero General Public License v3.0
1.92k stars 235 forks source link

[V5.2] PHP linters did not run locallly with PHP 8 #1060

Closed llaville closed 2 years ago

llaville commented 2 years ago

Describe the bug When you run MegaLinter locally with docker images, on source code installed for PHP 8, all PHP linters did not run correctly

Expected behavior Be able to specify which PHP built-in version (7.4 or 8.0) to run for each PHP linter (PHPStan, PHPCS, Psalm, PHPLint)

Screenshots

Results of phpstan linter (version 1.2.0)
See documentation on https://megalinter.github.io/descriptors/php_phpstan/
-----------------------------------------------

❌ [ERROR] for workspace /tmp/lint
Linter raw log:
PHP Fatal error:  Composer detected issues in your platform: Your Composer dependencies require a PHP version ">= 8.0.0". You are running 7.4.26. in /tmp/lint/vendor/composer/platform_check.php on line 24
Fatal error: Composer detected issues in your platform: Your Composer dependencies require a PHP version ">= 8.0.0". You are running 7.4.26. in /tmp/lint/vendor/composer/platform_check.php on line 24

Additional context

----------------------------------------------------------------------------------------------------
-------------------------------------------- MegaLinter --------------------------------------------
----------------------------------------------------------------------------------------------------
 - Image Creation Date: 2021-12-03T06:10:37Z
 - Image Revision: e770f95f
 - Image Version: beta

Run with following command : sudo rm -rf report/ && time docker run -v $(pwd):/tmp/lint -w $(pwd) megalinter/megalinter-php:beta

llaville commented 2 years ago

PHP 8 support for PHP built-in linter only, was introduced in MegaLinter since https://github.com/megalinter/megalinter/issues/788

llaville commented 2 years ago

Work around is possible if you remove vendor/ and composer.lock on PHP 8 installation and install source code again with PHP 7.4

llaville commented 2 years ago

This is also the reason, why I always use a PHP 7.4 matrix only to run MegaLinter on GitHub Workflows, like https://github.com/llaville/php-compat-info/blob/6.0/.github/workflows/php-tests.yaml#L20-L31

nvuillam commented 2 years ago

You know PHP much better than me... (not coded in PHP for 15 years ^^) do you have a proposition to solve this issue ?

llaville commented 2 years ago

Solution found, PR is in way ! Now time for explains

Solution is universal for all linters (PHP is just an example) that want to manage multiple version and switch from one to another at runtime. Its name => update-alternatives in package dpkg (https://pkgs.alpinelinux.org/contents?file=update-alternatives&path=&name=dpkg&branch=edge&repo=main&arch=x86)

A brief summary for those who don't know yet what is it

Define multiple runtime alternatives of a same executable. In our case: php

update-alternatives --install /usr/bin/php php /usr/bin/php7 100
update-alternatives --install /usr/bin/php php /usr/bin/php8 10

I gave highest priority (100) to PHP7 to keep current compatibility (PHP 7 standard in MegaLinter v5), then PHP 8 (10) See future PR with php.megalinter-descriptor.yml

Now, at runtime, we will use the PRE-COMMANDs MegaLinter feature:

If we want to switch to PHP 8 for PHP built-in linter

PHP_BUILTIN_PRE_COMMANDS:
    - command: "update-alternatives --set php /usr/bin/php8"
      cwd: root

Caution In some case it's not enough, when we use other PHP linters like PHPStan. In this case we have to switch this linter like that:

PHP_PHPSTAN_PRE_COMMANDS:
    - command: "update-alternatives --set php /usr/bin/php8"
      cwd: root

Test protocol was long, but I've run lot of test to check it's OK. Next some of them ... will follows

llaville commented 2 years ago

Checks if your source code was installed on platform PHP 7 or PHP 8.

compatinfo_composer_php74

On platform PHP 8, it will look like (php version depending on source code installed, of course)

compatinfo_composer_php80

On standard MegaLinter without PRE-COMMANDs switch we could have something like that:

compatinfo_ml-beta_php74

And with the switch to PHP 8 with PHP_BUILTIN_PRE_COMMANDS, we could have something like that

compatinfo_ml-alpha_php80

Now, with special case like PHPStan on platform PHP 8, when switch with PHP_BUILTIN_PRE_COMMANDS is not enough :

compatinfo-php8_phpstan_php74_issue-2

You must have to use the switch for this linter like :

PHP_PHPSTAN_PRE_COMMANDS:
    - command: "update-alternatives --set php /usr/bin/php8"
      cwd: root

And all goes right

nvuillam commented 2 years ago

Great analysis and soution :)

I would go even further:

We would obvisouly need to update core architecture to do that, so python reads some new descriptor yaml properties to know what to do... :)

llaville commented 2 years ago

Great analysis and soution :)

I would go even further:

  • is there a generic way to detect PHP version used in a repo ?

    • if yes, what about managing to automate all that without user having to define PRE_COMMANDS ? :)
    • if not.... same, but using some variable PHP_VERSION ? :)

We would obvisouly need to update core architecture to do that, so python reads some new descriptor yaml properties to know what to do... :)

I've tried to add a PHP_VERSION variable and do it with python, but it didn't work, so I've let down. And suggest only this solution.

nvuillam commented 2 years ago

@llaville what did not work ?

It could be done by defining a generic GenericPhpLinter class and a method before_lint_files :)

image

llaville commented 2 years ago

@nvuillam Will have a look on this approach tomorrow

llaville commented 2 years ago

@nvuillam Just tried with following code, but can't build locally the docker image. Sorry, but I'm not much free time today. Perharps in end of day, otherwise it will be later.

code

dockerbuild

nvuillam commented 2 years ago

If you can't build the image, CI can do it for you :p

llaville commented 2 years ago

@nvuillam Now I've be able to build the container, based on fresh main branch copy (v5.3+), I don't see any changes. How is it supposed to work ? File name is megalinter/linters/PhpLinter.py, and when I run the new container, I don't see any trace of "PHP before_lint_file" in log (see source code in screenshot above) Even when I set PHP_VERSION: "8" in a .meta-linter.yml config file of one of my PHP project file

llaville commented 2 years ago

Even, if I remove the if condition that seems unuseful here, I don't see anything. Source code is:

#!/usr/bin/env python3
"""
Use PHP to lint php files
https://www.php.net
"""
import logging
import os
import subprocess

import megalinter

class PhpLinter(megalinter.Linter):

    # To execute before linting files
    def before_lint_files(self):
        php_version = config.get("PHP_VERSION", "7")
        if php_version == "7":
            return
        pre_command = f"update-alternatives --set php /usr/bin/php{php_version}"
        logging.debug("PHP before_lint_files: " + pre_command)
        process = subprocess.run(
            pre_command,
            stdout=subprocess.PIPE,
            stderr=subprocess.STDOUT,
            shell=True,
        )
        return_code = process.returncode
        return_stdout = megalinter.utils.decode_utf8(process.stdout)
        logging.debug(f"{return_code} : {return_stdout}")
nvuillam commented 2 years ago

You code seems good :) Is it in a branch ? Did you add the class property in the descriptor on each php linter block ?

llaville commented 2 years ago

I just realized that I missed it ... building container in progress

llaville commented 2 years ago

Tests in progress ...

llaville commented 2 years ago

Results is strange megalinter.log file ended with


+----MATCHING LINTERS--+----------+----------------+------------+
| Descriptor | Linter  | Criteria | Matching files | Format/Fix |
+------------+---------+----------+----------------+------------+
| PHP        | phpstan | .php     | project        | no         |
| PHP        | phplint | .php     | project        | no         |
+------------+---------+----------+----------------+------------+

PHP: ['phpstan']
PHP: ['phplint']

No console reporter output ...

llaville commented 2 years ago

I'll push it to a branch, and I'll let CI do the job. We can see if it's my env that is not correct, or anything else

llaville commented 2 years ago

New branch is php_version_switch_v2

llaville commented 2 years ago

I've been able to run in PARALLEL: false mode but not the default PARALLEL: true

It seems that running multiple PHP linters in parallel with setting update-alternatives does not work very well

llaville commented 2 years ago

Ok it's working except error raised by update-alternatives due to conflict parallel tasks, but that do the job.

For example with phplint and phpstan

PHP: ['phpstan']
PHP: ['phplint']
PHP before_lint_files: update-alternatives --set php /usr/bin/php8
PHP before_lint_files: update-alternatives --set php /usr/bin/php8
0 : update-alternatives: using /usr/bin/php8 to provide /usr/bin/php (php) in manual mode

[phpstan] command: ['phpstan', 'analyse', '--no-progress', '--no-ansi', '--memory-limit', '1G', '-c', '/tmp/lint/.github/linters/phpstan.neon.dist']
[phpstan] CWD: /tmp/lint
2 : update-alternatives: using /usr/bin/php8 to provide /usr/bin/php (php) in manual mode
update-alternatives: error: unable to install '/var/lib/dpkg/alternatives/php.dpkg-tmp' as '/var/lib/dpkg/alternatives/php': No such file or directory

[phplint] command: ['phplint', '-c', '/tmp/lint/.github/linters/.phplint.yml']
[phplint] CWD: /tmp/lint
[phplint] result: 0 phplint 3.0 by overtrue and contributors.
llaville commented 2 years ago

Tested with phplint, phpstan, and even with psalm

psalm_414

Good new with psalm since v4.14: we know which PHP target is

nvuillam commented 2 years ago

That's strange... even with parallel mode, linters from same descriptor are supposed to be run one after another, to avoid such collisions... maybe before_lint_files is called too early in the process, and not really just before linting files :/

llaville commented 2 years ago

👍 But even with this error

2 : update-alternatives: using /usr/bin/php8 to provide /usr/bin/php (php) in manual mode update-alternatives: error: unable to install '/var/lib/dpkg/alternatives/php.dpkg-tmp' as '/var/lib/dpkg/alternatives/php': No such file or directory

All run fines in PHP 8 mode.