(explicitly) find the executable before running run_cmd

[majidaldo]

resolves #3035

Checklist

• I have signed the CLA
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
  ran test_system_client.py
• I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

[jtcohen6]

Thanks for this @majidaldo!

• Can you confirm that this change resolves git.bat not picked up on dbt deps #3035 for you when running dbt deps locally?
• It looks like there are two failing unit tests: test__executable_does_not_exist and test__not_exe. My guess here is that these tests are expecting an ExecutableError instead of a CommandError.
• Changelog: By "dbt next," we just mean the next (not-yet-released) version of dbt, which right now is dbt 0.20.0 (Release TBD). Could you add an entry beneath "Fixes" or "Under the hood" (whichever feels more appropriate) describing the change, with links to the issue and this PR in parentheses, and also add yourself to the list of Contributors?

@kwigley I'm going to tag you as a reviewer for a second (better-informed) pair of eyes. The proposed change is quite narrow, and I don't think it raises any security implications.

[kwigley]

👍 glad this was an easy fix, just asking to update the relevant exception raised but looks good otherwise

[kwigley]

whoops sorry! I meant to request changes

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Majid alDosari.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email [email protected]
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Majid alDosari.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email [email protected]
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[majidaldo]

• Can you confirm that this change resolves git.bat not picked up on dbt deps #3035 for you when running dbt deps locally?

Yes. I created a git.bat to wrap git.exe with "C:\path\to\git.exe" %* and put it in my current directory and dept deps worked. When I change it to "C:\path\to\notgit.exe" %* it expectedly errors.

[jtcohen6]

@majidaldo Thanks for updating! Could you add yourself to the list of Contributors in the Changelog as well?

In the CI, it looks like there's a new minor flake8 (style) error:

core/dbt/clients/system.py:412:80: E501 line too long (93 > 79 characters)

It also looks like these unit tests (test__executable_does_not_exist and test__not_exe) are still failing. In particular, even though it's now correctly raising an ExecutableError, the expected words 'path' and 'permissions' are not present in the returned error message.

In the CI runs, I'm seeing error messages like:

executable for /tmp/tmpi8g4qwbh/does_not_exist not found.: "/tmp/tmpi8g4qwbh/does_not_exist"
executable for /tmp/tmp1xj494qp/empty_file not found.: "/tmp/tmp1xj494qp/empty_file"

Versus the expected error messages on POSIX:

Could not find command, ensure it is in the user's PATH
User does not have permissions for this command

Or Windows:

Could not find command, ensure it is in the user's PATH and that the user has permissions to run it

The latter error messages do seem a bit more specific and helpful. I'm wondering if the if not exe_path case, instead of adding its own custom error handling, should either shell out to _interpret_oserror or pass (?) and allow a more specific error to happen further down the code path. What do you think?

[majidaldo]

@majidaldo Thanks for updating! Could you add yourself to the list of Contributors in the Changelog as well?

In the CI, it looks like there's a new minor flake8 (style) error:

core/dbt/clients/system.py:412:80: E501 line too long (93 > 79 characters)

It also looks like these unit tests (test__executable_does_not_exist and test__not_exe) are still failing. In particular, even though it's now correctly raising an ExecutableError, the expected words 'path' and 'permissions' are not present in the returned error message.

In the CI runs, I'm seeing error messages like:

executable for /tmp/tmpi8g4qwbh/does_not_exist not found.: "/tmp/tmpi8g4qwbh/does_not_exist"
executable for /tmp/tmp1xj494qp/empty_file not found.: "/tmp/tmp1xj494qp/empty_file"

Versus the expected error messages on POSIX:

Could not find command, ensure it is in the user's PATH
User does not have permissions for this command

Or Windows:

Could not find command, ensure it is in the user's PATH and that the user has permissions to run it

The latter error messages do seem a bit more specific and helpful. I'm wondering if the if not exe_path case, instead of adding its own custom error handling, should either shell out to _interpret_oserror or pass (?) and allow a more specific error to happen further down the code path. What do you think?

I'm afraid my 'scope' is to just offer my help for a small fix. I'd really need to dig into the design of dbt to give you a full answer.

[jtcohen6]

I'm afraid my 'scope' is to just offer my help for a small fix. I'd really need to dig into the design of dbt to give you a full answer.

@majidaldo Got it — would you like us to take the code from here? I'm hopeful there's just a little bit of cleanup left.

[majidaldo]

I'm afraid my 'scope' is to just offer my help for a small fix. I'd really need to dig into the design of dbt to give you a full answer.

@majidaldo Got it — would you like us to take the code from here? I'm hopeful there's just a little bit of cleanup left.

Please. That would be the quickest way to improve the code. I hope I pointed out the usefulness of using which which (hah) helps with diagnosing problems on both linux and windows.

Thanks for the great work!

[jtcohen6]

@majidaldo Could you take #3299 for a spin? I think I managed to accomplish the same functional change, while preserving the original error messages. I'd like to confirm with you first, however, before merging!

[majidaldo]

@majidaldo Could you take #3299 for a spin? I think I managed to accomplish the same functional change, while preserving the original error messages. I'd like to confirm with you first, however, before merging!

Yup! dbt deps:

• w/o git gives the expected message
• w/ git.exe works
• w/ git.bat works

thanks!