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

feat(utils): Support non-exe commands on Windows #2019

Merged
merged 1 commit into from
Dec 22, 2020

Conversation

andytom
Copy link
Member

@andytom andytom commented Dec 20, 2020

Description

Have added support to the utils::exec_cmd to allow it to execute
commands that are not .exe on Windows. Have also added a timer to
measure how long a command took to execute.

Motivation and Context

Closes #983
Closes #2018

Screenshots (if appropriate):

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

Have added support to the `utils::exec_cmd` to allow it to execute
commands that are not `.exe` on Windows. Have also added a timer to
measure how long a command took to execute.
@andytom andytom requested a review from a team December 20, 2020 18:41
@andytom andytom marked this pull request as ready for review December 20, 2020 21:51
Copy link
Member

@davidkna davidkna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Elixir version shows up with this on windows (It takes 250ms, but it's also this slow when not run with starship).

@@ -243,14 +244,31 @@ pub fn wrap_seq_for_shell(

fn internal_exec_cmd(cmd: &str, args: &[&str]) -> Option<CommandOutput> {
log::trace!("Executing command {:?} with args {:?}", cmd, args);
match Command::new(cmd).args(args).output() {

let full_path = match which::which(cmd) {
Copy link
Member

@davidkna davidkna Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth it to build a map of all the exetuables in the PATH in the future, to avoid having to walk the whole PATH on each invocation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that might be something to look at in the future or maybe something we can get added to the upstream library.

@andytom andytom merged commit 85de6df into starship:master Dec 22, 2020
@andytom andytom deleted the feat/better_cmd_discovery branch December 22, 2020 16:44
@andytom
Copy link
Member Author

andytom commented Dec 22, 2020

Thanks for the review @davidkna

chipbuster pushed a commit to chipbuster/starship that referenced this pull request Dec 31, 2020
Have added support to the `utils::exec_cmd` to allow it to execute
commands that are not `.exe` on Windows. Have also added a timer to
measure how long a command took to execute.
chipbuster pushed a commit to chipbuster/starship that referenced this pull request Jan 14, 2021
Have added support to the `utils::exec_cmd` to allow it to execute
commands that are not `.exe` on Windows. Have also added a timer to
measure how long a command took to execute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants