SkipLintProcess: Pass script by file-name instead of code #178
Conversation
When running the tests locally, I realized that patch 146 did not actually work correctly on Windows. This commit adds test runs against Windows in CI on a limited number of PHP versions to prevent this kind of issue going unnoticed for future PRs. Note: the lowest PHP version I can get a running build on is PHP 5.5. This is related to SSL transport issues with Packagist with old Composer versions (which are needed for old PHP versions).
There was a problem hiding this comment.
@staabm That's awesome investigative work between you and @cmb69. Thank you both so much !
I've also confirmed the fix via a test run on a local Windows machine.
Unfortunately, as the project is also released as a PHAR file, the solution needs more work as realpath() doesn't work for files packaged in a PHAR. (I'm not sure about is_file())
src/Process/SkipLintProcess.php
Outdated
|
|
||
| if (!$script) { | ||
| if (!is_file($scriptPath)) { |
There was a problem hiding this comment.
| if (!is_file($scriptPath)) { | |
| if (!@is_file($scriptPath)) { |
I think it would make sense to silence a potential warning coming from is_file() as the exception being thrown makes the warning redundant.
src/Process/SkipLintProcess.php
Outdated
| @@ -23,15 +23,12 @@ class SkipLintProcess extends PhpProcess | |||
| public function __construct(PhpExecutable $phpExecutable, array $filesToCheck) | |||
| { | |||
| $scriptPath = __DIR__ . '/../../bin/skip-linting.php'; | |||
There was a problem hiding this comment.
Brain fart (untested): changing this line to the below and removing the call to realpath() might fix the issue with the PHAR ?
| $scriptPath = __DIR__ . '/../../bin/skip-linting.php'; | |
| $scriptPath = dirname(dirname(__DIR__)) . '/bin/skip-linting.php'; |
There was a problem hiding this comment.
found a working solution at https://php-tips.readthedocs.io/en/latest/tips/run_any_phar_file.html
clxmstaab
force-pushed
the
simpl
branch
2 times, most recently
from
eb58bc8 to
2b3c305
Compare
| $script = str_replace('<?php', '', $script); | ||
|
|
||
| $parameters = array('-d', 'display_errors=stderr', '-r', $script); | ||
| $parameters = array('-d', 'display_errors=stderr', '-r', $code); | ||
| parent::__construct($phpExecutable, $parameters, implode(PHP_EOL, $filesToCheck)); |
There was a problem hiding this comment.
btw: I wonder why this skip-linting logic is implemented via subprocess at all. I think parallel-lint would be faster when it just invokes the logic directly.
but thats a story for a future PR.
There was a problem hiding this comment.
I've been wondering the same, but that was before my time, so will need some history digging (in the old repo) to see if we can figure out the reason. If there is none, I'd welcome a change for this.
|
its hard to debug the github action stuff when every change requires an approval :-/ |
Indeed, but that can't be helped for now. I think it's the My own "hack" to get around the approval thingie is to:
But I doubt I'm telling you anything new ? |
I am currently looking into it ;) |
|
@jrfnl please re-approve |
|
Since the test matrix was adjusted the required checks for the repo need to be adjusted in the repo settings |
Fixes the windows test-failure indentified in #170 by
passing the script path toinclude instead of passing the code as a string which get mangled by laterSkipLintProcessescapeshellarg().see php/php-src#16147 (comment) for a in-detail analysis why this did not work on windows - kudos to Christoph M. Becker for the root cause analysis.
closes #170