-
Notifications
You must be signed in to change notification settings - Fork 63
Improve how trogon is calling the command with arguments #107
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
base: main
Are you sure you want to change the base?
Conversation
I have tested this PR and it solve my issues with running commands on Windows. Especially #67. |
I am waiting on the merge as well |
@daneah by any chance can you review and comment my PR? |
I'll try to review in depth soon @bigbirdcode — I'm working through finalizing my talk about our internal CLI tooling for PyTexas this coming weekend. |
import sys | ||
from types import ModuleType | ||
|
||
import oslex |
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.
I'm not confident we should be taking on this dependency—it appears not to have been updated in some time, nor with many stars on GitHub. It is a small enough footprint that we should perhaps bring what we need of its functionality here into this package directly.
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.
In my opinion oslex
(with mslex
) should be in the standard library. It is a huge pain that standard library only contains shlex, ignoring all the Windows users.
oslex
is not frequently changed, because it does not need to be. It is a wrapper above shlex
and mslex
. And since it is a relative new package it is not really known yet. But again, it is a must. If you don't trust third party dependencies, then I can fix the exact version.
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.
Reminder here, poetry.lock
was not updated, I don't have tooling for that.
This PR also solves #46 |
Same, been for a while, currently I am monkey patching instead, and would really rather not |
Co-authored-by: Dane Hillard <github@danehillard.com>
Sorry for not replying for a long time, I have changed job, I had other priorities in my life. But now I fixed the finding, thank you for that, and I'm ready to do other changes if needed. You can also take into account that Python 3.14 has changed the |
Are there any news on this? Is there something I can do to help bring this into trogons release? |
This is taking an awfully long time to merge in. I am not trying to be impatient, but it's been months, and this is a small easy to understand change. This change fixes a lot of things that cause trogon to flat out not work on windows. Which is a lot of my users for my applications. |
Is there anything I can help to support a new release? My previous team use my fork and branch to install Trogon on Windows. But this is sad... |
This PR tries to resolve several problems. All the changes are small and connected, that is why I opened just one PR.
Please review it commit by commit.
Fixes:
#104 - In
run_command.py
file theUserCommandData.to_cli_args()
now contains string conversion.I have not added conversion to
_to_cli_args()
as it was already too complex.#105 -
shlex
replaced tooslex
, for the need of Windows users.I have added to
pyproject.toml
.TODO:
poetry.lock
was not updated, I don't have tooling for that.#106 - To run well and wait for the output on Windows I have added
sys.exit(subprocess.call(..., shell=True))
as the pair ofos.execvp()
call.Note:
shell=True
was needed for me.#81 , #70 , #67 , #65 - I try to fix all these. In the file
detect_run_string.py
the functiondetect_run_string()
returns a short version of the command, while for execution we need the full version. So I have added a functionexact_run_commands()
. I have also modified how the command is called introgon.py
and replacedos.execvp()
toos.execv()
.Tested:
-m
, outside of a venv with direct path to Python.exe, and with installed app-m
, outside of a venv with direct path to python3I have not tested with pipx or such cases. Please tell me of more modification is needed.