Nothing Special   »   [go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

support linting multiple files at once using php -l #10024

Closed
staabm opened this issue Nov 30, 2022 · 10 comments
Closed

support linting multiple files at once using php -l #10024

staabm opened this issue Nov 30, 2022 · 10 comments
Labels
Feature Status: Needs Triage

Comments

@staabm
Copy link
Contributor
staabm commented Nov 30, 2022 •
edited
Loading

Description

would it be possible to support passing multiple files to php -l, e.g. php -l file1.php file2.php file3.php?

my request is motivated by the fact the current linting tools are slow because of the overhead involved to lint every single file using a single process, so e.g. proc_open dominates the whole process.

see this example blackfire profile, where I lint a project with 2890 files which takes 1min 14secs:

tested with on windows11 - but we see also unnecessary slowdown within github action jobs (not that big as in windows)

PHP 8.1.2 (cli) (built: Jan 19 2022 10:13:52) (NTS Visual C++ 2019 x64)
Copyright (c) The PHP Group
Zend Engine v4.1.2, Copyright (c) Zend Technologies

grafik

if we could e.g. lint 2 or 3 files at a time we could easily cut this processing time in half or even more.
@cmb69
Copy link
Member
cmb69 commented Nov 30, 2022

See https://bugs.php.net/40077. Might be doable and useful.

@staabm
Copy link
Contributor Author
staabm commented Nov 30, 2022 •
edited
Loading

Btw: I find it pretty suprising that it takes over 1 minute on a 10 core cpu windows laptop to lint the same project, which takes 15 seconds on a 2 core github action job (with 10 processes in parallel)

@cmb69
Copy link
Member
cmb69 commented Nov 30, 2022

I find it pretty suprising that it takes over 1 minute on a 10 core cpu windows laptop to lint the same project, which takes 15 seconds on a 2 core github action job (with 10 processes in parallel)

I assume this is due to file system performance (which is slow on Windows, and multiple cores won't help with that).

@staabm
Copy link
Contributor Author
staabm commented Dec 1, 2022

I assume this is due to file system performance

I am not that much into the details regarding windows internals.
maybe its worth to profile how expensive process creation on windows is and whether there are some low hanging fruits to get.

I am not at all a C expert though.

thanks for all the feedback until now.

@cmb69
Copy link
Member
cmb69 commented Dec 1, 2022

maybe its worth to profile how expensive process creation on windows is

Oh, right, creating new PHP processes is also very slow, especially if there are many DLLs to load (part of that problem is ASLR). We have been able to considerably speed up our test runs in CI by only loading the extensions which are really required for a single test case. Anyhow, this issue would be solved for the linting case, if we support multiple files to be linted at once.

But there is still the bad file system performance, see https://bugs.php.net/80695.

@staabm
Copy link
Contributor Author
staabm commented Dec 1, 2022 •
edited
Loading

this issue would be solved for the linting case

could you give me a few hints where I can start looking into it?

I would start here

do you agree we would just allow to support a fixed number of additional arguments, or do you think we should support "n" arguments (no matter how much given)?

@cmb69
Copy link
Member
cmb69 commented Dec 1, 2022

Yes, these are the correct places (note, though, that the cgi and litespeed SAPIs also support linting). And see:

php-src/sapi/cli/php_cli.c

Lines 967 to 974 in d6ac533

case PHP_MODE_LINT:
if (php_lint_script(&file_handle) == SUCCESS) {
zend_printf("No syntax errors detected in %s\n", php_self);
EG(exit_status) = 0;
} else {
zend_printf("Errors parsing %s\n", php_self);
EG(exit_status) = 255;
}

I think supporting an arbitrary amount of files makes the most sense, but that might get somewhat ugly, since currently reading from STDIN is supported, and we should not allow multiple files for other operations.

@KapitanOczywisty
Copy link

@cmb69 Would be sensible to implement glob patterns here? I'd expect that in most cases you want to lint everything.

@cmb69
Copy link
Member
cmb69 commented Dec 1, 2022

Would be sensible to implement glob patterns here? I'd expect that in most cases you want to lint everything.

I'd rather avoid that. Instead, users could do something like ls | xargs php -l (or build the command line by other means).

@ghnp5
Copy link
ghnp5 commented Feb 16, 2023

Hey

Is it just me, or is it muuuuch slower to lint since PHP 8.2?

I have a 128GB RAM server with NVMe, and a very good CPU, and until PHP 8.1, it used to be fast enough (~1 minute) to run the lint.

Since I upgraded to PHP 8.2, it takes ~2 or 3 minutes to run the same lint across all files.

Anyway - regarding running multiple lints, I know this isn't the answer to this Feature Request, but I've just found this:
https://packagist.org/packages/overtrue/phplint
https://github.com/overtrue/phplint

which can hopefully speed up the releases.

nielsdos added a commit to nielsdos/php-src that referenced this issue Feb 26, 2023
@nielsdos
Implement phpGH-10024: support linting multiple files at once using p…
b765aaa 
…hp -l

This is supported in both the CLI and CGI modes. For CLI this required
little changes.

For CGI, the tricky part was that the options parsing happens inside the
loop. This means that options passed after the -l flag were previously
simply ignored. As we now re-enter the loop we would parse the options
again, and if they are handled but don't set the script name, then CGI
will think you want to read from standard in. To keep the same "don't
parse options" behaviour I simply wrapped the options handling inside an
if.
@nielsdos nielsdos mentioned this issue Feb 26, 2023
Closed
nielsdos added a commit to nielsdos/php-src that referenced this issue Feb 26, 2023
@nielsdos
Implement phpGH-10024: support linting multiple files at once using p…
feb4eec 
…hp -l

This is supported in both the CLI and CGI modes. For CLI this required
little changes.

For CGI, the tricky part was that the options parsing happens inside the
loop. This means that options passed after the -l flag were previously
simply ignored. As we now re-enter the loop we would parse the options
again, and if they are handled but don't set the script name, then CGI
will think you want to read from standard in. To keep the same "don't
parse options" behaviour I simply wrapped the options handling inside an
if.
@staabm staabm mentioned this issue Apr 7, 2023
Closed
nielsdos added a commit to nielsdos/php-src that referenced this issue Jun 29, 2023
@nielsdos
Implement phpGH-10024: support linting multiple files at once using p…
084f6ab 
…hp -l

This is supported in both the CLI and CGI modes. For CLI this required
little changes.

For CGI, the tricky part was that the options parsing happens inside the
loop. This means that options passed after the -l flag were previously
simply ignored. As we now re-enter the loop we would parse the options
again, and if they are handled but don't set the script name, then CGI
will think you want to read from standard in. To keep the same "don't
parse options" behaviour I simply wrapped the options handling inside an
if.
@staabm staabm mentioned this issue Dec 14, 2023
Closed
@staabm staabm mentioned this issue Oct 2, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature Status: Needs Triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement GH-10024: support linting multiple files at once using php -l nielsdos/php-src
4 participants
@staabm @cmb69 @KapitanOczywisty @ghnp5