-
Notifications
You must be signed in to change notification settings - Fork 23
SkipLintProcess: Pass script by file-name instead of code #178
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brain fart (untested): changing this line to the below and removing the call to realpath() might fix the issue with the PHAR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
found a working solution at https://php-tips.readthedocs.io/en/latest/tips/run_any_phar_file.html
eb58bc8 to
2b3c305
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-/
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 grep which is causing the problem, but haven't had the time to properly debug this today (I did push one commit to #170 to hopefully improve things already)
My own "hack" to get around the approval thingie is to:
- Turn on actions in my own fork
- Comment out the
brancheslines for theon - push(in a separate commit which I can easily remove) - Often also slim down the matrix to just run the problem builds I want to focus on (and one previously passing one safeguard that I'm not breaking it)
- And then just hack away at it with debug statements etc until I get it working (preferably in a non-PR branch so people watching don't get an email about every push)
But I doubt I'm telling you anything new ?
@jrfnl please re-approve
a210713 to
ea19c13
Compare
382776f to
aa0e4a0
Compare
247087f to
f20e77c
Compare
Since the test matrix was adjusted the required checks for the repo need to be adjusted in the repo settings
Uh oh!
There was an error while loading. Please reload this page.
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 laterSkipLintProcess(削除ここまで)escapeshellarg().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