-
Notifications
You must be signed in to change notification settings - Fork 482
feat!: use executables on $PATH
#738
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
Conversation
Jint-lzxy
left a comment
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 am not very supportive of making this change b/c we ought to use the pre-compiled executables for host_progs due to the -m <mod> flag (usually -m pip <arguments> is used for installing dependencies, and users who have installed many versions of Python(3) will encounter environment issues)
-m mod : run library module as a script (terminates option list)|
But using this change solves the problem of adaptation to different platforms. You cannot assume that the command exists in the specified directory, especially for the Windows platform. |
|
It should be up to the user to ensure that the environment variables are correct, rather than specifying a path that cannot be changed. |
|
IMO issuing errors early (executable not found) is way more intuitive (and debuggable) than getting Say if a user gets |
|
I don't have any doubts, but I still think that it would be better to use environment variables, one is more general, it can be cross-platform, and the other is that users should be responsible for their own environment variables, after all, setting environment variables is a basic operation for developers. |
Agree with you on this. We shall wait for other reviewers to check this PR, and if most of us are happy with this idea, we can also make more changes and remove several unnecessary conditionals 👍 |
|
For versatility, just use executable name is better because we provides cross-platform experience in fact. Approve these changes. |
$PATH
Jint-lzxy
left a comment
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
* fix: use commands in environment variables instead of absolute paths * fixup! fix: use commands in environment variables instead of absolute paths --------- Co-authored-by: Jint-lzxy <50296129+Jint-lzxy@users.noreply.github.com>
* fix: use commands in environment variables instead of absolute paths * fixup! fix: use commands in environment variables instead of absolute paths --------- Co-authored-by: Jint-lzxy <50296129+Jint-lzxy@users.noreply.github.com>
|
side note: i hard coded the path just to get a faster startup time. |
* fix: use commands in environment variables instead of absolute paths * fixup! fix: use commands in environment variables instead of absolute paths --------- Co-authored-by: Jint-lzxy <50296129+Jint-lzxy@users.noreply.github.com>
No description provided.