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

feature #4024 added a list-files command #5390

Merged
merged 28 commits into from Apr 17, 2021

Conversation

clxmstaab
Copy link
Contributor

@clxmstaab clxmstaab commented Dec 23, 2020

as discussed in #4024 this PR proposes a list-files command, which from a user-perspective can be used to utilize xargs to get some basic parallelization into php-cs-fixer.

this is especially interessting for CI build cases, in which the php-cs-fixer builtin caching mechanism doesn't work (e.g. because git does use the correct file modification stamps on clone or the CI build is running within a readonly environment).

example usage:

vendor/bin/php-cs-fixer list-files | xargs -n 10 -P 8 vendor/bin/php-cs-fixer fix --config=.php_cs.dist --using-cache=no

-n defines how many files a single subprocess process
-P defines how many subprocesses the shell is allowed to spawn for parallel processing (usually similar to the number of CPUs your system has)

see xargs help page: https://wiki.ubuntuusers.de/xargs/

as can be seen in #4024 (comment) you might get a perf boost by a factor of 3-6x depending on the number of files beeing fixed, number of CPUs etc.

this isn't the most elegant way of how parallelzation can be applied on the php-cs-fixer case, but its a really simple one which works with very simple means and therefore has a rather low entry barrier.
you don't need fancy php extension or any rather complicated setup. simple bash means are enought to get a decent perf boost.
also its pretty simple from php-cs-fixer perspective to keep the maintenance burden as low as possible.

in case you would consider this change acceptable, I am willing to provide automated tests, docs and everything else required and missing atm.

any opinions?

@staabm staabm mentioned this pull request Dec 23, 2020
@clxmstaab clxmstaab changed the title feature #4024 added a list-files command feature #4024 added a list-files command Dec 23, 2020
@keradus
Copy link
Member

keradus commented Dec 24, 2020

in which the php-cs-fixer builtin caching mechanism doesn't work (e.g. because git does use the correct file modification stamps on clone or the CI build is running within a readonly environment)

Just a note, Cache mechanism relies on checksum of file, not it's timestamp, so this is not the case.
Indeed, if you have readonly mechanism, then you cannot use the cache easily

@mfn
Copy link

mfn commented Dec 24, 2020

One use-case which wasn't mentioned, in which actually triggered my curiosity in finding a solution speed up non-cached runs: changing the rules.

Doesn't sound like much, but: if you consider a large code base where, as I demonstrated in #4024 (comment) , a run takes 1m40s: that's no found to try out rules and analyze the impact. In fact I was going through lots-o-rules and couldn't bear it after 2 or 3 changes, so I came up with this hack with xargs, because ~30s is acceptable.

Thanks!

@clxmstaab clxmstaab marked this pull request as draft January 5, 2021 09:10
@keradus
Copy link
Member

keradus commented Jan 8, 2021

@clxmstaab , please ping me if you would need another round of review!
eager to merge this feature when it's ready ;)

@mfn
Copy link

mfn commented Jan 9, 2021

If for supporting it all it takes is the new command as is + additional the cache check

maybe we should have a switch to filter-out files that do not require a fixing (based on cache) ?

My modest feedback: I would like to see this PR focus only on that one feature: return the list of files as it would be found by the finder.

Let the connection to the cache be a follow-up change, if it's strongly desired.

The feature will be useful "as is" and it keeps the scope / review process easier.

My 2c.

/** @var SplFileInfo $file */
foreach ($finder as $file) {
if ($file->isFile()) {
$output->writeln(escapeshellarg($file->getRelativePathname()));
Copy link
Contributor Author

@clxmstaab clxmstaab Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while real-world testing, I got 'sometimes' a error

...
'tests/Tokenizer/TransformersTest.php'
'tests/ToolInfoTest.php'
'tests/UtilsTest.php'
'tests/WhitespacesFixerConfigTest.php'
'tests/WordMatcherTest.php'
PHP Fatal error:  Uncaught Error: Call to undefined method SplFileInfo::getRelativePathname() in /mnt/c/dvl/GitHub/PHP-CS-Fixer/src/Console/Command/ListFilesCommand.php:86
Stack trace:
#0 /mnt/c/dvl/GitHub/PHP-CS-Fixer/vendor/symfony/console/Command/Command.php(255): PhpCsFixer\Console\Command\ListFilesCommand->execute()
#1 /mnt/c/dvl/GitHub/PHP-CS-Fixer/vendor/symfony/console/Application.php(971): Symfony\Component\Console\Command\Command->run()
#2 /mnt/c/dvl/GitHub/PHP-CS-Fixer/vendor/symfony/console/Application.php(290): Symfony\Component\Console\Application->doRunCommand()
#3 /mnt/c/dvl/GitHub/PHP-CS-Fixer/src/Console/Application.php(93): Symfony\Component\Console\Application->doRun()
#4 /mnt/c/dvl/GitHub/PHP-CS-Fixer/vendor/symfony/console/Application.php(166): PhpCsFixer\Console\Application->doRun()
#5 /mnt/c/dvl/GitHub/PHP-CS-Fixer/php-cs-fixer(102): Symfony\Component\Console\Application->run()
#6 {main}
  thrown in /mnt/c/dvl/GitHub/PHP-CS-Fixer/src/Console/Command/ListFilesCommand.php on line 86

its strange, since it works for nearly all files, but only a few error.

does the sfFinder sometimes return a \SplFileInfo and sometimes a Symfony\Component\Finder\SplFileInfo?
(getRelativePathname is only defined on the symfony-SplFileInfo class)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's probably not the only place, but looks like Finder can return built-in \SplFileInfo too
https://github.com/symfony/finder/blob/5.x/Finder.php#L660

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you reproduce the issue with unit test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worst case scenario, simply check if object we got implements sf-file or \file and use one path or another

@clxmstaab clxmstaab marked this pull request as ready for review January 15, 2021 08:35
@clxmstaab
Copy link
Contributor Author

any feedback on the current status @keradus ?

@keradus keradus changed the base branch from 2.17 to master January 24, 2021 21:33
@keradus
Copy link
Member

keradus commented Apr 13, 2021

looking good, just some minors in CodeReview, CI should be fixed, and I'm really curious about that relative path

@clxmstaab
Copy link
Contributor Author

clxmstaab commented Apr 15, 2021

first - thanks for your feedback @keradus - appreciate that!

I think this should be good to go.

the github actions are pretty slow right now, so the results will show whether I am right.
the static analysis errors looks unrelated to me.

does the sfFinder sometimes return a \SplFileInfo and sometimes a Symfony\Component\Finder\SplFileInfo?
(getRelativePathname is only defined on the symfony-SplFileInfo class)

I did some path gymnastics and I think this resolved the Problem. the new version works with what is available in \SplFileInfo, and thats ok for the sfSplFileInfo because it extends \SplFileInfo.

feedback welcome.

@clxmstaab
Copy link
Contributor Author

looking at CI, it looks to me that the errors are unrelated

doc/usage.rst Outdated

.. code-block:: console

$ php php-cs-fixer.phar list-files | xargs -n 10 -P 8 php-cs-fixer fix
Copy link
Member

@keradus keradus Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to count the files and provide =$files/$cpu to single job?

Copy link
Member

@keradus keradus Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to provide multiple files to fix command, you need to specify config file (ref #4279 (comment) )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to count the files and provide =$files/$cpu to single job?

not sure what you mean or what you are after?

to provide multiple files to fix command

hmm does this mean we should make the config a required argument instead of a option?

@coveralls
Copy link

coveralls commented Apr 15, 2021

Coverage Status

Coverage increased (+0.01%) to 91.906% when pulling ed65a9e on clxmstaab:list-files-cmd into 72dd15d on FriendsOfPHP:master.

@clxmstaab
Copy link
Contributor Author

i merged the fix, can you rebase to prove it was only related to it?

it seems this works now, thx

@keradus
Copy link
Member

keradus commented Apr 16, 2021

as I was afraid, after the fix for main branch was merged, this PR is still red - Windows failing, 5.6 failing, fabbot failing. can you take a look?

@clxmstaab
Copy link
Contributor Author

fixed the windows builds, thx for the headsup.

the macos buid failure seems unrelated

doc/usage.rst Outdated Show resolved Hide resolved
doc/usage.rst Outdated Show resolved Hide resolved
@keradus
Copy link
Member

keradus commented Apr 17, 2021

Really nice work, @clxmstaab !

@keradus
Copy link
Member

keradus commented Apr 17, 2021

Thank you @clxmstaab.

@keradus keradus merged commit 3799baf into PHP-CS-Fixer:master Apr 17, 2021
@staabm staabm mentioned this pull request Apr 18, 2021
@clxmstaab clxmstaab deleted the list-files-cmd branch April 19, 2021 07:21
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.

None yet

6 participants