Stop reusing core's commit-code-check.sh in favor of 4 simple commands

Wim Leers → credited omkar.podey → .

Well well … as promised during yesterday's meeting, I looked into solving this challenge in core. So I found @phenaproxima's ✨ Make it possible for contrib modules to reuse core's commit-code-check.sh Needs work which is blocked on 📌 Fix cspell use: specify globRoot and always pass --root to cspell Fixed . So I spent yesterday solving that.

In doing so, I discovered Drupal core had not been running PHPStan/cspell/… either!

I reported that very late in my day yesterday and overnight it was fixed in 🐛 Fix commit-code-check.sh on DrupalCI Fixed .

Now that is out of the way, AU's script is failing in a different way:

00:01:48.682 modules/contrib/automatic_updates/commit-code-check.sh --drupalci
00:01:48.764 warning: unable to access 'modules/contrib/automatic_updates/.gitignore': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/tests/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/src/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/scripts/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/package_manager/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/css/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/config/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/automatic_updates_extensions/': Permission denied

@omkar.podey is investigating this at #3341224-14: Always catch \Throwable, not \Exception and always pass the old exception when re-throwing. → .

I left out the juiciest bit in #4:

00:01:48.682 modules/contrib/automatic_updates/commit-code-check.sh --drupalci
00:01:48.764 warning: unable to access 'modules/contrib/automatic_updates/.gitignore': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/tests/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/src/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/scripts/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/package_manager/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/css/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/config/': Permission denied
00:01:48.764 warning: could not open directory 'modules/contrib/automatic_updates/automatic_updates_extensions/': Permission denied
00:02:06.564 
1/3 /var/www/html/.gitattributes 680.12ms
00:02:07.536 
2/3 /var/www/html/README.md 101.96ms
00:02:07.639 
3/3 /var/www/html/composer.json 144.73ms
00:02:07.785 CSpell: Files checked: 3, Issues found: 0 in 0 files
00:02:07.812 
00:02:07.812 CSpell: passed
00:02:07.812 
00:02:07.812 ----------------------------------------------------------------------------------------------------
00:02:07.812 
00:02:07.812 Running PHPStan on *all* files.
00:03:29.644 
00:03:29.650  [OK] No errors                                                                 
00:03:29.650 
00:03:29.693 
00:03:29.693 PHPStan: passed
00:03:29.693 
00:03:29.693 ----------------------------------------------------------------------------------------------------
00:03:36.946 .........W...E........E.EEEEEEEBuild timed out (after 110 minutes). Marking the build as aborted.
01:50:00.428 Build was aborted
01:50:00.429 Archiving artifacts

→ the problems in #4 are just the early ones, PHPSTan eventually runs forever and that's really the problem here.

I don't see yet how 🐛 Fix commit-code-check.sh on DrupalCI Fixed could cause this.

Still investigating over at 📌 Always catch \Throwable, not \Exception, and pass the old exception when re-throwing. Fixed …

Update: set -xev helps us understand this:

00:01:58.137 + DEPENDENCIES_NEED_INSTALLING=0
00:01:58.137 # Ensure PHP development dependencies are installed.
00:01:58.137 # @todo https://github.com/composer/composer/issues/4497 Improve this to
00:01:58.137 #  determine if dependencies in the lock file match the installed versions.
00:01:58.137 #  Using composer install --dry-run is not valid because it would depend on
00:01:58.137 #  user-facing strings in Composer.
00:01:58.137 if ! [[ -f '/var/www/html/vendor/bin/phpcs' ]]; then
00:01:58.137   printf "Drupal's PHP development dependencies are not installed. Run 'composer install' from the root directory.\n"
00:01:58.137   DEPENDENCIES_NEED_INSTALLING=1;
00:01:58.137 fi
00:01:58.137 + [[ -f /var/www/html/vendor/bin/phpcs ]]
00:01:58.137 modules/contrib/automatic_updates/commit-code-check.sh: 186: [[: not found
00:01:58.137 + printf Drupal's PHP development dependencies are not installed. Run 'composer install' from the root directory.\n
00:01:58.137 + DEPENDENCIES_NEED_INSTALLING=1

— https://dispatcher.drupalci.org/job/drupal8_contrib_patches/145897/console

→ it appears that phpcs is not installed? That seems odd.

also: I was wrong in #6: that output shows pretty clearly that phpstan finishes just fine; the next thing to run is phpcs, and that's what fails/runs forever! Somehow 😳 So that could be explained by the above. Although it's then pretty mysterious why the error would have be swallowed prior to the set -xev…

🕵️‍♀️

While breaking my head over this, here's an alternative attempted work-around…

That worked for phpcs and phpstan, but not yet cspell and eslint. Adding debug output…

Ah, so /var/www/html/core is not the core subdirectory of core…

More debug output. This is very confusing.

Updating status per #3341224-16: Always catch \Throwable, not \Exception and always pass the old exception when re-throwing. → .

Looks like cd results are lost in this context … so let's put it all in a single line instead then.

In the proposed resolution, we would have committed a copy of commit-code-check.sh that hardcodes DrupalCI's absolute paths, so it still would not have worked locally…

Besides, no module is even remotely as big as Drupal core. So the many optimizations in that script to only check the files that have changed is really premature optimization/unjustifiable complexity.

So … we can make this 💯 times simpler.

#15 works.

1. It runs phpcs and finds no violations 👍
2. It catches all phpstan violations that 🐛 Fix PHPStan violations (happened because PHPStan code checks stopped running…) Fixed is fixing. 👍
3. It finds spelling violations because I am not yet incorporating our dictionary. 👍
4. It finds a weird problem in our drupalci.yml file — this is the sole thing that probably needs double-checking — running the same command locally definitely detects a wrongly formatted routes YAML file!

Need confirmation that we want to go this direction. Only cspell dictionary and the drupalci.yml problems remain to be solved, other than that this is already ready!

Discussed with @tedbow and @phenaproxima. They're on board! 🥳

Let's finish this.

30 minutes searching for this … 😬 cspell's DX can be improved quite a bit still…

Thanks to https://github.com/streetsidesoftware/cspell/pull/966, we can easily make cspell discover our dictionary 👍

1. Stop spellchecking pcre.ini and README.md, neither of which will go into core anyway
2. Add --no-progress so cspell's output is not ridiculously overwhelming and detailed.

Moved it all into a script: scripts/commit-code-check.sh. And calling that. So that delivers on @tedbow's original hope/vision 😊

Make --drupalci flag work and chmod a+x.

Whoops, that $FINAL_STATUS was not yet being respected.

Simplify by introducing a few bash functions, that also allows for much better consistency!

#24 looks like this:

Fix the phpcs violation that landed last night.

Merging in 🐛 Fix PHPStan violations (happened because PHPStan code checks stopped running…) Fixed by doing curl https://git.drupalcode.org/project/automatic_updates/-/merge_requests/708.diff | git apply -3v && git ci -nam "Yash's phpstan fixes" and generating a new patch — this should be green 🤓

Looks like @yash still has some work cut out for him!

Please commit #26 first right now, so that @yash.rode can continue in 🐛 Fix PHPStan violations (happened because PHPStan code checks stopped running…) Fixed tomorrow :)

(If you prefer, you could choose to disable the phpstan checking by commenting that one line in there, but I think that's not really helping us.)

For the committer:

+++ b/scripts/commit-code-check.sh
@@ -0,0 +1,120 @@
+print_title "[1/4] PHPCS"
+$CORE_DIRECTORY/vendor/bin/phpcs $MODULE_DIRECTORY -ps --standard="$CORE_DIRECTORY/core/phpcs.xml.dist"
+print_results $? "PHPCS"
+
+print_title "[2/4] PHPStan"
+php -d apc.enabled=0 -d apc.enable_cli=0 $CORE_DIRECTORY/vendor/bin/phpstan analyze --no-progress --configuration="$CORE_DIRECTORY/core/phpstan.neon.dist" $MODULE_DIRECTORY
+print_results $? "PHPStan"
+
+print_title "[3/4] CSpell"
+cd $CORE_DIRECTORY/core && yarn run -s spellcheck --no-progress --root $MODULE_DIRECTORY -c .cspell.json "**" && cd -
+print_results $? "CSpell"
+
+print_title "[4/4] eslint:yaml"
+cd $CORE_DIRECTORY/core && yarn eslint --resolve-plugins-relative-to . --ext .yml $MODULE_DIRECTORY && cd -
+print_results $? "eslint:yaml"
+print_separator

This is really how we run tests.

Everything else is for:
- documentation
- supporting a --drupalci flag, just like core's script
- setting up variables for colored output, just like core's script
- existing the script with a computed "final status" just like the core script
- discovering the core directory

… but with far less repetition thanks to print_seprator, print_title and print_results helper functions

@Wim Leers 🙏🙏🙏🙏🙏🙏🙏🙏

I committed this with the phpstan part commented out but just pushed a change to 🐛 Fix PHPStan violations (happened because PHPStan code checks stopped running…) Fixed that uncomments this

Working on possible follow-up

That is one weird follow-up 🤣 Where are you going with that?!

Extracted some of the key simplifications this has compared to core's script into a core patch: 📌 Simplify commit-code-check.sh: reduce repetition Needs work .

This was blocking the path to core for sure!

Automatically closed - issue fixed for 2 weeks with no activity.