Skip to content

Conversation

bigbirdcode
Copy link

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 the UserCommandData.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 to oslex, 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 of os.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 function detect_run_string() returns a short version of the command, while for execution we need the full version. So I have added a function exact_run_commands(). I have also modified how the command is called in trogon.py and replaced os.execvp() to os.execv().

Tested:

  • Windows, from venv with and without -m, outside of a venv with direct path to Python.exe, and with installed app
  • Linux, from venv with and without -m, outside of a venv with direct path to python3

I have not tested with pipx or such cases. Please tell me of more modification is needed.

@bigbirdcode bigbirdcode changed the title Improve cli args Improve how trogon is calling the command with arguments Feb 1, 2025
@bissonex
Copy link

I have tested this PR and it solve my issues with running commands on Windows. Especially #67.

@Mythical-Github
Copy link

I am waiting on the merge as well

@bigbirdcode
Copy link
Author

@daneah by any chance can you review and comment my PR?

@daneah
Copy link
Collaborator

daneah commented Apr 6, 2025

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
Copy link
Collaborator

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.

Copy link
Author

@bigbirdcode bigbirdcode Jun 28, 2025

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.

Copy link
Author

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.

@Tijoxa
Copy link

Tijoxa commented Jun 26, 2025

This PR also solves #46
I am waiting for the merge.

@Mythical-Github
Copy link

Same, been for a while, currently I am monkey patching instead, and would really rather not

Co-authored-by: Dane Hillard <github@danehillard.com>
@bigbirdcode
Copy link
Author

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 argparse module, it now has a _prog_name function, with the same goal, to find the right executable name.

@42sol-eu
Copy link

42sol-eu commented Jul 25, 2025

Are there any news on this?
I have the issue calling os.execvp on Mac M1 with Python 3.11
Sound that there would be a solution to fix this that is pending a merge.

Is there something I can do to help bring this into trogons release?

@Mythical-Github
Copy link

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.

@bigbirdcode bigbirdcode requested a review from daneah August 30, 2025 15:56
@bigbirdcode
Copy link
Author

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...

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.

6 participants