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

Optimize php linting command to speed up CI builds on TravisCI #403

Merged
merged 32 commits into from Mar 15, 2019

Conversation

Projects
None yet
3 participants
@andygrunwald
Copy link
Collaborator

andygrunwald commented Jan 26, 2019

Work in Progress: This PR is work in progress and should not be merged right now. The basic idea/goal is to speed up the CI build times.

Now we run/What it does:

  • PHP Linting: 8 checks in parallel
  • PHP Linting: Only output PHP errors, if a file is valid it won't be outputted anymore
  • Bundle phpcs calls to reduce startup time

@andygrunwald andygrunwald requested a review from M4LuZ Jan 26, 2019

@andygrunwald andygrunwald changed the title Optimize php linting command to speed up CI builds on TravisCI [WIP] Optimize php linting command to speed up CI builds on TravisCI Jan 26, 2019

Merge branch 'master' into speed-up-ci-build
* master:
  Import: OPTIMIZE the correct table please (add prefix).
  Import: Move DBPrimaryKeys initialization upwards to fix Countable error $DBPrimaryKeys is used later on in a count() that throws a warning in newer PHP releases as the initialization is skipped in "rewrite" cases.
  Issue #380: Drop support for PHP 7.0
  Fix PHP7.2 warning when trying to count non-countable objects PHP7.2 is more strict on what can be count()'ed and what not. This first checks if the object is an array and only then does the counting; fixing #370 while at it.

@andygrunwald andygrunwald changed the title [WIP] Optimize php linting command to speed up CI builds on TravisCI Optimize php linting command to speed up CI builds on TravisCI Jan 31, 2019

@andygrunwald andygrunwald added the WIP label Jan 31, 2019

andygrunwald and others added some commits Jan 31, 2019

Tournament2: Add missing quotes
.. to silence PHP warnings.
@M4LuZ

M4LuZ approved these changes Feb 4, 2019

@M4LuZ

This comment has been minimized.

Copy link
Collaborator

M4LuZ commented Feb 4, 2019

@andygrunwald: Is this still WIP or ready to be merged?

@andygrunwald

This comment has been minimized.

Copy link
Collaborator Author

andygrunwald commented Feb 4, 2019

Still work in progress.
Until now i didn't made big improvements here.

@M4LuZ

This comment has been minimized.

Copy link
Collaborator

M4LuZ commented Feb 5, 2019

Until now i didn't made big improvements here.

I think we will also not see any improvements here as the resources granted per instance are limited and I think that parallel execution may actually slow things down a bit.
What may make sense instead is to restrict linting via phpcs to the set of changed files as these are the only ones that contain changes to be checked
Syntax checks would need to run at least once on all files (due to potential dependencies in non-modified files) before merging, but may also be reduced to just the changed files for immediate checks.

No clue how to implement that though ;)

M4LuZ added a commit that referenced this pull request Mar 8, 2019

comitting a few files with issues
testing issue detection with travis if only changed files are checked.

refs #403
Added --diff-filter=ACM as added files were ignored and deleted files…
… caused issues

C should not be necessary as the source file of the copy should already be ok as it is in the repo
@M4LuZ

This comment has been minimized.

Copy link
Collaborator

M4LuZ commented Mar 8, 2019

@andygrunwald please have a look here.
Was able to achieve a large speedup by just selecting the modified files.
We do not need to check existing files, as they are per definition OK if they are in the master.
I thought that it still makes sense that the php syntax runs on every file but it appears that no dependencies are checked, thus also here only changes need to be validated.

I ran a few test cases in the branch 'test-travis-speedup' and noted the following:

git diff does not return added files with the current call, so that still needs fixing here.
I'm not that confident with neither Travis nor Git, so would need your help here!

M4LuZ added some commits Mar 8, 2019

@M4LuZ

This comment has been minimized.

Copy link
Collaborator

M4LuZ commented Mar 8, 2019

OK, I give up.
Thought that using $TRAVIS_COMMITwould help, but Travis creates a merge commit and that is what is contained in that variable. But somehow I'm unable to obtain the changeset from there.
Help pl0x!

@M4LuZ M4LuZ closed this Mar 8, 2019

@M4LuZ M4LuZ reopened this Mar 8, 2019

@M4LuZ

This comment has been minimized.

Copy link
Collaborator

M4LuZ commented Mar 8, 2019

File selection seems OK (new and modified files checked, deleted files ignored)
But the verification now fails to fail, seems the return code is not validated correctly

M4LuZ and others added some commits Mar 8, 2019

Last try (again).
seems that the for loop returns OK and thus the build is still OK even on failures.
Replaced with xargs, copy & pasted together
-0 for xargs was senseless
also added echo for the changed files again
@M4LuZ

This comment has been minimized.

Copy link
Collaborator

M4LuZ commented Mar 8, 2019

yeah, about that. A grep has to be added to the first call to make sure that only php files are passed for php -l

@M4LuZ

This comment has been minimized.

Copy link
Collaborator

M4LuZ commented Mar 8, 2019

OK, seems we are now good on all points.
There is still a minor cosmetic issue as grep "\.php$" exits with an error if there is no change to a php file, but directly the next line forces the job to be OK for exactly that case.

@andygrunwald would be happy if you give it a spin and add your two cents here ;)

@andygrunwald

This comment has been minimized.

Copy link
Collaborator Author

andygrunwald commented Mar 14, 2019

From a quick look, it looks good. Thanks for putting the hard work in it. I hope this speeds up the process a lot :)

One side option:
Once we modify a big file that is not fully PSR-1/PSR-2 compliant, the check PHP_CodeSniffer check will fail. The reason is, that there are no more "exclusions" anymore like before. Before we applied several exclusions on modules and more.

This will be maybe hard if someone touches a big file.
I am personally fine with it. Many modern IDEs have an "auto format feature" for PSR-1 or PSR-2.

IMO: Go ahead, merge it.

@M4LuZ

This comment has been minimized.

Copy link
Collaborator

M4LuZ commented Mar 15, 2019

Once we modify a big file that is not fully PSR-1/PSR-2 compliant, the check PHP_CodeSniffer check will fail.

Yes, but fixing that should be fast and most of the files should be OK already.
We may want to exclude the line length again, but that can be handled separately.

I hope this speeds up the process a lot :)

It does, see the CI tests in the branch test-travis-speedup
Basically this now takes the VM boot time (around a minute) + a few seconds.
So there is not much left to optimize besides caching the composer downloads somehow.

But merging this for now

@M4LuZ M4LuZ merged commit b75af3c into master Mar 15, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@M4LuZ M4LuZ deleted the speed-up-ci-build branch Mar 15, 2019

@M4LuZ M4LuZ removed the WIP label Mar 15, 2019

@andygrunwald andygrunwald referenced this pull request Mar 23, 2019

Open

Update wizard.php #441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.