-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix console appearing on daemon in Windows #6450
Conversation
_popen( | ||
cmd, env=env, creationflags=creationflags, startupinfo=startupinfo | ||
).communicate() | ||
_popen(cmd, env=env, creationflags=creationflags, startupinfo=startupinfo) |
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.
We just need to spawn it, I don't think we need to wait for it.
cc @mattseddon |
97fd9a5
to
bfb26e6
Compare
Sorry, could not see the console on my computer. Maybe because it is too fast? |
Did you check on master branch? |
Yes. I'm on 2.6.0+a285d2 |
Do you have analytics enabled? |
|
bfb26e6
to
e83da2c
Compare
main_entrypoint = os.path.join( | ||
os.path.abspath(os.path.dirname(__file__)), "__main__.py" | ||
) | ||
prefix += [main_entrypoint] |
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.
@iterative/dvc, do you know a better way to get the executable? In Windows, the sys.argv[0]
is .env/Scripts/dvc
which is just a link to .env/Scripts/dvc.exe
built from the console_scripts
. So, we were calling it as /path/to/python .env/Scripts/dvc
which is not a python file and the daemon was failing on Windows.
In Linux/MacOS, based on how you invoke, it's either __main__.py
or the .env/bin/dvc
.
I think invoking __main__.py
directly is the best approach here, but I'd like to hear the alternatives.
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.
@skshetry It is also important to not forget about our exe binary built by pyinstaller, IIRC __main__.py
is not present there, but I might be mistaken.
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.
@efiop, It's inside the if not is_binary():
block as before. ;)
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.
@skshetry Ah, sorry, totally blinded that one. 🤦
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.
LGTM
Trying to fix #6449.
Would be great to have confirmation from someone using Windows.