-
Notifications
You must be signed in to change notification settings - Fork 20
feat: User friendly error when executable not found #177
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
feat: User friendly error when executable not found #177
Conversation
Also checks the result of `which`, just in case there's an alias that's pointing to nowhere.
…found Also checks the result of `which`, just in case there's an alias that's pointing to nowhere. Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…om/jblagden/openjd-adaptor-runtime-for-python into better_error_subprocess_executable
Fixed an always-true type check Removed an explicit throw of un-caught exception types
…om/jblagden/openjd-adaptor-runtime-for-python into better_error_subprocess_executable
| except FileNotFoundError as fnf_error: | ||
| # In ManagedProcess we prepend the executable to the list of arguments before creating a LoggingSubprocess | ||
| executable = args[0] | ||
| exe_path = shutil.which(executable) |
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.
If we're using shutil.which to resolve the program, it will be better to do this before we call subprocess.Popen, and then modify args[0] to contain the result of that. We found that on Windows, subprocess.Popen and shutil.which can get different answers, so the approach you've implemented could make inaccurate error messages.
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.
Interesting - do you recall how we might be able to re-create that? It might be worth a unit test to save future folks from a similar pitfall. I'll get a tweak up as soon as I can - likely tomorrow morning.
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.
What a mistake it was to commit to a time I'd get back to this!
I think I've got it all straightened out now though, let me know what you think when you have a chance.
…om/jblagden/openjd-adaptor-runtime-for-python into better_error_subprocess_executable
…om/jblagden/openjd-adaptor-runtime-for-python into better_error_subprocess_executable
Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
|
|
Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…om/jblagden/openjd-adaptor-runtime-for-python into better_error_subprocess_executable Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…iption#159) Updates the requirements on [python-semantic-release](https://github.com/python-semantic-release/python-semantic-release) to permit the latest version. - [Release notes](https://github.com/python-semantic-release/python-semantic-release/releases) - [Changelog](https://github.com/python-semantic-release/python-semantic-release/blob/master/CHANGELOG.md) - [Commits](python-semantic-release/python-semantic-release@v9.12...v9.14) --- updated-dependencies: - dependency-name: python-semantic-release dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Signed-off-by: Joel Wong <127782171+joel-wong-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Signed-off-by: client-software-ci <129794699+client-software-ci@users.noreply.github.com> Signed-off-by: Joel Wong <127782171+joel-wong-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…bDescription#171) Signed-off-by: Karthik Bekal Pattathana <133984042+karthikbekalp@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Signed-off-by: client-software-ci <129794699+client-software-ci@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…found Also checks the result of `which`, just in case there's an alias that's pointing to nowhere. Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Fixed an always-true type check Removed an explicit throw of un-caught exception types Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Signed-off-by: Joel Wong <127782171+joel-wong-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…ription#169) Signed-off-by: Joel Wong <127782171+joel-wong-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…escription#182) Signed-off-by: Joel Wong <127782171+joel-wong-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…iption#181) Updates the requirements on [python-semantic-release](https://github.com/python-semantic-release/python-semantic-release) to permit the latest version. - [Release notes](https://github.com/python-semantic-release/python-semantic-release/releases) - [Changelog](https://github.com/python-semantic-release/python-semantic-release/blob/master/CHANGELOG.rst) - [Commits](python-semantic-release/python-semantic-release@v9.14...v9.20) --- updated-dependencies: - dependency-name: python-semantic-release dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Charles Moore <122481442+moorec-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…JobDescription#168) Updates the requirements on [hatch](https://github.com/pypa/hatch) to permit the latest version. - [Release notes](https://github.com/pypa/hatch/releases) - [Commits](pypa/hatch@hatch-v1.13.0...hatch-v1.14.0) --- updated-dependencies: - dependency-name: hatch dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Charles Moore <122481442+moorec-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…Description#175) Updates the requirements on [ruff](https://github.com/astral-sh/ruff) to permit the latest version. - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](astral-sh/ruff@0.8.0...0.9.1) --- updated-dependencies: - dependency-name: ruff dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Charles Moore <122481442+moorec-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…ools (OpenJobDescription#186) Signed-off-by: Charles Moore <122481442+moorec-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…obDescription#185) Updates the requirements on [psutil](https://github.com/giampaolo/psutil) to permit the latest version. - [Changelog](https://github.com/giampaolo/psutil/blob/master/HISTORY.rst) - [Commits](giampaolo/psutil@release-6.1.0...release-7.0.0) --- updated-dependencies: - dependency-name: psutil dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Charles Moore <122481442+moorec-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…escription#184) Updates the requirements on [black](https://github.com/psf/black) to permit the latest version. - [Release notes](https://github.com/psf/black/releases) - [Changelog](https://github.com/psf/black/blob/main/CHANGES.md) - [Commits](psf/black@24.1a1...25.1.0) --- updated-dependencies: - dependency-name: black dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…iption#183) Updates the requirements on [python-semantic-release](https://github.com/python-semantic-release/python-semantic-release) to permit the latest version. - [Release notes](https://github.com/python-semantic-release/python-semantic-release/releases) - [Changelog](https://github.com/python-semantic-release/python-semantic-release/blob/master/CHANGELOG.rst) - [Commits](python-semantic-release/python-semantic-release@v9.20...v9.21) --- updated-dependencies: - dependency-name: python-semantic-release dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…bDescription#189) Updates the requirements on [ruff](https://github.com/astral-sh/ruff) to permit the latest version. - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](astral-sh/ruff@0.9.0...0.11.0) --- updated-dependencies: - dependency-name: ruff dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…penJobDescription#190) Updates the requirements on [pytest-cov](https://github.com/pytest-dev/pytest-cov) to permit the latest version. - [Changelog](https://github.com/pytest-dev/pytest-cov/blob/master/CHANGELOG.rst) - [Commits](pytest-dev/pytest-cov@v6.0.0...v6.1.1) --- updated-dependencies: - dependency-name: pytest-cov dependency-version: 6.1.1 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Signed-off-by: Cody Edwards <edwards@amazon.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
OpenJobDescription#192) Updates the requirements on [pytest-timeout](https://github.com/pytest-dev/pytest-timeout) to permit the latest version. - [Commits](pytest-dev/pytest-timeout@2.3.0...2.4.0) --- updated-dependencies: - dependency-name: pytest-timeout dependency-version: 2.4.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Signed-off-by: Charles Moore <122481442+moorec-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Updates the requirements on [hatch-vcs](https://github.com/ofek/hatch-vcs) to permit the latest version. - [Release notes](https://github.com/ofek/hatch-vcs/releases) - [Changelog](https://github.com/ofek/hatch-vcs/blob/master/HISTORY.md) - [Commits](ofek/hatch-vcs@v0.4.0...v0.5.0) --- updated-dependencies: - dependency-name: hatch-vcs dependency-version: 0.5.0 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Updates the requirements on [pytest-xdist](https://github.com/pytest-dev/pytest-xdist) to permit the latest version. - [Release notes](https://github.com/pytest-dev/pytest-xdist/releases) - [Changelog](https://github.com/pytest-dev/pytest-xdist/blob/master/CHANGELOG.rst) - [Commits](pytest-dev/pytest-xdist@v3.6.0...v3.7.0) --- updated-dependencies: - dependency-name: pytest-xdist dependency-version: 3.7.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Updates the requirements on [pytest](https://github.com/pytest-dev/pytest) to permit the latest version. - [Release notes](https://github.com/pytest-dev/pytest/releases) - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst) - [Commits](pytest-dev/pytest@8.3.0.dev0...8.4.0) --- updated-dependencies: - dependency-name: pytest dependency-version: 8.4.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Updates the requirements on [mypy](https://github.com/python/mypy) to permit the latest version. - [Changelog](https://github.com/python/mypy/blob/master/CHANGELOG.md) - [Commits](python/mypy@v1.15.0...v1.16.0) --- updated-dependencies: - dependency-name: mypy dependency-version: 1.16.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Signed-off-by: Morgan Epp <60796713+epmog@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
* Changes to the introduction are to clarify the purpose of this library, making it easier to understand what features it provides. * Moved the content about using the library from README.md into a docs directory. With the re-organization, there's a place to add new content like worked examples or more details about interface features the library supports. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Signed-off-by: Charles Moore <122481442+moorec-aws@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…scription#202) This is a proposal to try a very different adaptor interface than is currently used. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Updates the requirements on [python-semantic-release](https://github.com/python-semantic-release/python-semantic-release) to permit the latest version. - [Release notes](https://github.com/python-semantic-release/python-semantic-release/releases) - [Changelog](https://github.com/python-semantic-release/python-semantic-release/blob/master/CHANGELOG.rst) - [Commits](python-semantic-release/python-semantic-release@v9.21...v10.0.2) --- updated-dependencies: - dependency-name: python-semantic-release dependency-version: 10.0.2 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
…om/jblagden/openjd-adaptor-runtime-for-python into better_error_subprocess_executable Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
Signed-off-by: Justin Blagden <jblagden@users.noreply.github.com>
|



Fixes: #113
What was the problem/requirement? (What/Why)
When the adaptor fails to find the set executable, it'll fail with
[WinError 2] The system cannot find the file specified, which without any context doesn't set users up to sort the issue out.What was the solution? (How)
This change adds user friendly messages about what's failing and possible reasons why.
What is the impact of this change?
Users will see more logging when the adaptor fails to find the executable it has been set to run.
How was this change tested?
Currently a gap in this PR - I'm not sure where to start testing if a file does or does not exist and modifying the environment in a test.
See DEVELOPMENT.md for information on running tests.
Yes, to failures on TestIntegrationClientInterface, TestIntegrationLoggingSubprocess, and TestDaemonMode:
I'm working in the Windows subsystem for Linux which might be the cause of the failing timeouts but I'm not sure about the SIGTERM issue. I'm hoping the github runners shed a little light on what's happening.
This still needs a test for the case when an alias is set but the executable isn't found. I'm not certain that it's a usefully different code-path. Removing that may be better.
Was this change documented?
Yes.
Is this a breaking change?
No.
Does this change impact security?
No.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.