Skip to content

Conversation

@jblagden
Copy link
Contributor

@jblagden jblagden commented Jan 27, 2025

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.

  • Have you run the unit tests?

Yes, to failures on TestIntegrationClientInterface, TestIntegrationLoggingSubprocess, and TestDaemonMode:

FAILED test/openjd/adaptor_runtime_client/integ/test_integration_client_interface.py::TestIntegrationClientInterface::test_graceful_shutdown - AssertionError: assert 'Received SIGTERM signal.' in ''
FAILED test/openjd/adaptor_runtime/integ/process/test_integration_logging_subprocess.py::TestIntegrationLoggingSubprocess::test_stop_process[StopProcessWhenSIGTERMFails] - Failed: Timeout >5.0s
FAILED test/openjd/adaptor_runtime/integ/background/test_background_mode.py::TestDaemonMode::test_shutdown - psutil.TimeoutExpired: timeout after 1 seconds (pid=11312)

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.

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 <[email protected]>
@jblagden jblagden changed the title Better error subprocess executable feat! User friendly error when executable not found Jan 27, 2025
@jblagden jblagden marked this pull request as ready for review February 3, 2025 19:59
@jblagden jblagden requested a review from a team as a code owner February 3, 2025 19:59
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)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@jblagden jblagden changed the title feat! User friendly error when executable not found feat: User friendly error when executable not found Feb 19, 2025
@sonarqubecloud
Copy link

@sonarqubecloud
Copy link

jblagden and others added 11 commits August 21, 2025 13:18
…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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Justin Blagden <[email protected]>
Signed-off-by: Joel Wong <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
Signed-off-by: client-software-ci <[email protected]>
Signed-off-by: Joel Wong <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
Signed-off-by: client-software-ci <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
…found

Also checks the result of `which`, just in case there's an alias that's pointing to nowhere.

Signed-off-by: Justin Blagden <[email protected]>
Fixed an always-true type check

Removed an explicit throw of un-caught exception types

Signed-off-by: Justin Blagden <[email protected]>
joel-wong-aws and others added 26 commits August 21, 2025 13:44
…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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Charles Moore <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
…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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Charles Moore <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
…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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Charles Moore <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
…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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Charles Moore <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
…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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Justin Blagden <[email protected]>
…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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Justin Blagden <[email protected]>
…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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Justin Blagden <[email protected]>
…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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Justin Blagden <[email protected]>
Signed-off-by: Cody Edwards <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Justin Blagden <[email protected]>
Signed-off-by: Charles Moore <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
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] <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
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] <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
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] <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
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] <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
Signed-off-by: Morgan Epp <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
* 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 <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
…scription#202)

This is a proposal to try a very different adaptor interface than
is currently used.

Signed-off-by: Mark <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
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] <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
Signed-off-by: Justin Blagden <[email protected]>
@sonarqubecloud
Copy link

@jblagden jblagden closed this Aug 21, 2025
@jblagden jblagden deleted the better_error_subprocess_executable branch August 21, 2025 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: More informative error messages when a subprocess executable cannot be found.

8 participants