Skip to content
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

git.bat not picked up on dbt deps #3035

Closed
majidaldo opened this issue Jan 26, 2021 · 8 comments · Fixed by #3299
Closed

git.bat not picked up on dbt deps #3035

majidaldo opened this issue Jan 26, 2021 · 8 comments · Fixed by #3299
Labels
bug Something isn't working windows Everyone's favorite OS that's sometimes a little weird

Comments

@majidaldo
Copy link
Contributor

majidaldo commented Jan 26, 2021

ref. #1110

> dbt deps
Encountered an error:
Could not find command, ensure it is in the user's PATH: "git"

I have reasons to wrap my executables. On Windows I wrap my executables in .bat scripts (to get a similar effect to .sh). But then Popen( ['git'] ) doesn't work. Popen( ['git.bat']) works.

So the platform-independent way to get git to work is to use shutil.which to get the full path to the exe. Popen([ which('git') ], ) works.

@majidaldo majidaldo added bug Something isn't working triage labels Jan 26, 2021
@jtcohen6 jtcohen6 removed the triage label Jan 26, 2021
@jtcohen6
Copy link
Contributor

@majidaldo Just to clarify, you're encountering an error when you run dbt deps?

@majidaldo
Copy link
Contributor Author

@majidaldo Just to clarify, you're encountering an error when you run dbt deps?

ah sorry. yes.

@majidaldo majidaldo changed the title git.bat not picked up git.bat not picked up on dbt deps Jan 26, 2021
@jtcohen6
Copy link
Contributor

Ok! And if I understand your original message, are you saying that you're able to resolve the dbt deps error by changing line 420 below from cmd to shutil.which(cmd)?
https:/fishtown-analytics/dbt/blob/42a85ac39f34b058678fd0c03ff8e8d2835d2808/core/dbt/clients/system.py#L419-L424

@jtcohen6 jtcohen6 added the windows Everyone's favorite OS that's sometimes a little weird label Jan 28, 2021
@majidaldo
Copy link
Contributor Author

...well, depend on how you guys want to structure subprocess calls. The way you have it is that cmd is a list of strings (shell=False). So you'd only need to change the first element in the list: cmd --> [which(cmd[0])] + list(cmd[1:]).

@majidaldo
Copy link
Contributor Author

FYI, with shell=True, your wouldn't have to use which. You'd have to consider security implications but I think it's safe here.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 1, 2021

Ok, this one is a little bit beyond me. @kwigley When you have a moment, could I ask for your sense-check here?

@majidaldo
Copy link
Contributor Author

bump. this should be straightforward.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Mar 1, 2021

@majidaldo Is this a fix you'd be interested in contributing? I'd welcome a PR for it, if you don't mind explaining the system/security implications of the proposed changes.

majidaldo added a commit to majidaldo/dbt that referenced this issue Mar 1, 2021
majidaldo pushed a commit to majidaldo/dbt that referenced this issue Mar 8, 2021
jtcohen6 pushed a commit that referenced this issue Apr 27, 2021
jtcohen6 added a commit that referenced this issue May 20, 2021
* (explicitly) find the executable before running run_cmd

#3035

* fix undefined var

* use Executable to say exe not found and use full pth to exe

* changelog for #3035

* Nest shutil.which for better error msg

Co-authored-by: Majid alDosari <[email protected]>
Co-authored-by: Kyle Wigley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working windows Everyone's favorite OS that's sometimes a little weird
Projects
None yet
2 participants