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

run_custom_command: allow using interactive shell on unix #383

Merged
merged 2 commits into from Mar 17, 2023

Conversation

utkarshgupta137
Copy link
Contributor

Standards checklist:

  • The PR title is descriptive.
  • The code compiles (cargo build)
  • The code passes rustfmt (cargo fmt)
  • The code passes clippy (cargo clippy)
  • The code passes tests (cargo test)
  • Optional: I have tested the code myself
    • I also tested that Topgrade skips the step where needed

This allows loading environment & aliases from the user's rc files.

@reini-1 @ealap Please test once for any regressions.

@s34m
Copy link
Member

s34m commented Mar 14, 2023

The question is if we want to have that as an option in the argument string or create a new option which can be set to true or false. And if we add the option to source a .bashrc/.zshrc file before executing a command.

@utkarshgupta137
Copy link
Contributor Author

The benefit of having it as part of the argument string is that it can be toggled on a per-command basis. Otherwise, someone will inevitably run into the same problems with some other custom command

@s34m
Copy link
Member

s34m commented Mar 15, 2023

Lgtm, but could you write a text into the readme that's explains that this feature exists and how it works.

@utkarshgupta137
Copy link
Contributor Author

Lgtm, but could you write a text into the readme that's explains that this feature exists and how it works.

Is there an existing section on custom commands where I can add it?
I think the example in the config example is sufficient?

@s34m
Copy link
Member

s34m commented Mar 15, 2023

I would just add a little text like:

For running commands in an interactive shell, for example to source your shells rc file, you can ads -i in front of your custom command. But note that this requires the command to exit the shell correctly or else the shell will hang indefinitely.

@reini-1
Copy link

reini-1 commented Mar 16, 2023

why not use

"custom command" = "bash -i -c 'foo bar'"

for running a command with interactive shell?

Why is there an extra option or something else necessary if it is already possible to do it?

@reini-1
Copy link

reini-1 commented Mar 16, 2023

@utkarshgupta137 How can I test this? Where can I find the binaries?

@s34m
Copy link
Member

s34m commented Mar 16, 2023

why not use

"custom command" = "bash -i -c 'foo bar'"

for running a command with interactive shell?

Why is there an extra option or something else necessary if it is already possible to do it?

  1. Because the shell is automatically determined
  2. Because for the normal command -c shouldn't be included in the command string.

@utkarshgupta137
Copy link
Contributor Author

@utkarshgupta137 How can I test this? Where can I find the binaries?

You can clone my fork & then run cargo install --path .. If you don't have rust installed, use http://rustup.sh/.

@reini-1
Copy link

reini-1 commented Mar 16, 2023

1. Because the shell is automatically determined

"custom command" = "$SHELL -i -c 'foo bar'"

should also use the default shell or you can explicitly use another shell, e.g. zsh or fish or bash installed in another path for a single command.

2. Because for the normal command -c shouldn't be included in the command string.

For a normal command I would not want to depend on some shell functions or aliases. For non-interactive shells you maybe want the shell to startup fast and do not load something like a complicated prompt that shows git status, current time, weater, ...

I also do not know, if all shells support aliases/function.

But for me it is ok, as long as it is possible to not run interactive shells for custom commands.

@s34m
Copy link
Member

s34m commented Mar 17, 2023

@reini-1 by default if you just specify a command it doesn't load any aliases and functions. Those are only loaded when the shell is run interactively.

@ealap
Copy link

ealap commented Mar 17, 2023

@utkarshgupta137 @DottoDev, I just tried this and I can confirm that

  • without the -i string, the subshell spawned is on non-interactive mode.
  • with the -i string, the subshell spawn is interactive and resolves aliases and functions. However, issues identified from custom commands stopped and topgrade exited #380 still persists. Thus, the difference now is you are only hiding it behind an option.

In my opinion, I think that we should just stick to non-interactive mode and if users want to use their own aliases/functions, instruct them to source the script containing those aliases (this is what I do now).

"custom_cmd" = "source ~/.zsh_aliases; my_custom_command"

@s34m
Copy link
Member

s34m commented Mar 17, 2023

I mean sourcing it is always an option but I still like the idea of running it interactive if somebody needs it for anything.

@utkarshgupta137
Copy link
Contributor Author

Conceptually, I agree that an interactive shell shouldn't be used by an automated tool.
But I still think a simple & shell/system independent option is not a bad idea.

@ealap
Copy link

ealap commented Mar 17, 2023

Well I guess go for this then😉 We are in a better situation now that it is behind the -i string option. Maybe one of us can refine the solution in the future to fix the issue on zsh

@s34m
Copy link
Member

s34m commented Mar 17, 2023

This will be merged till a better option will maybe be available

@s34m s34m merged commit 907465f into topgrade-rs:master Mar 17, 2023
7 checks passed
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.

None yet

4 participants