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

fix(oh-my-zsh): fix remote oh-my-zsh issue #496

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

SteveLauC
Copy link
Member

@SteveLauC SteveLauC commented Jul 13, 2023

Standards checklist:

  • The PR title is descriptive.
  • I have read CONTRIBUTING.md
  • 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

If you developed a feature or a bug fix for someone else and you do not have the
means to test it, please tag this person here.

Closes #477


Root cause of that issue

See this comment

Solution

We judge if we are running under ssh through environment variables SSH_CLIENT and SSH_TTY, if yes, we spawn a zsh process and make zshrc sourced, then we extract the environment variable ZSH from this process, and set it in the topgrade process.

Fixed issues

  1. New "fatal" error with oh-my-zsh  #477
  2. If a user has a custom oh-my-zsh installation (not ~/.oh-my-zsh) on a remote machine and the user tries to update oh-my-zsh on that remote machine through topgrade, then this step will be skipped as environment variable ZSH is absent and the hardcoded fallback path (~/.oh-my-zsh) does not exist. This is pretty similar to [oh-my-zsh]$ZSH should be used to compose its install directory #426 but on a remote machine, this patch also fixes it.

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #496 (5e0dc1b) into master (9cd155f) will decrease coverage by 0.16%.
The diff coverage is 2.11%.

@@            Coverage Diff            @@
##           master    #496      +/-   ##
=========================================
- Coverage    3.50%   3.35%   -0.16%     
=========================================
  Files          27      31       +4     
  Lines        3141    4208    +1067     
=========================================
+ Hits          110     141      +31     
- Misses       3031    4067    +1036     
Impacted Files Coverage Δ
src/command.rs 0.00% <0.00%> (ø)
src/ctrlc/interrupted.rs 0.00% <ø> (ø)
src/error.rs 0.00% <0.00%> (ø)
src/execution_context.rs 0.00% <0.00%> (ø)
src/executor.rs 0.00% <0.00%> (ø)
src/main.rs 0.36% <0.00%> (-0.06%) ⬇️
src/report.rs 0.00% <0.00%> (ø)
src/runner.rs 0.00% <0.00%> (ø)
src/steps/containers.rs 0.00% <0.00%> (ø)
src/steps/emacs.rs 0.00% <0.00%> (ø)
... and 20 more

@SteveLauC
Copy link
Member Author

Friendly ping the guys in #477 @Auravendill @loehden @leoheck @ahuston-0

Here is a build for x86_64-unknown-linux-musl with this patch included, you can test it in your machines.

tip: put this binary to your remote hosts should be sufficient as the one on localhost is not affected

https://github.com/SteveLauC/topgrade/releases/tag/oh-my-zsh-remote

@Auravendill
Copy link

Here is a build for x86_64-unknown-linux-musl with this patch included, you can test it in your machines.

Is it okay to run the musl-Version on GNU/Linux (Debian)?

@SteveLauC
Copy link
Member Author

Here is a build for x86_64-unknown-linux-musl with this patch included, you can test it in your machines.

Is it okay to run the musl-Version on GNU/Linux (Debian)?

IMHO, yes. Actually, I build it for musl just for your Debian system, the libc on the Debian-based distro is relatively old, and the one on my host (Fedora) is quite new, a build for my host probably won't work on Debian

@Auravendill
Copy link

2023-07-13_20-36

I installed that version on the machine called "server" and as you can see, it was the only one, that worked. It seems like this does indeed fix the issue.

@loehden
Copy link

loehden commented Jul 14, 2023

Friendly ping the guys in #477 @Auravendill @loehden @leoheck @ahuston-0

Here is a build for x86_64-unknown-linux-musl with this patch included, you can test it in your machines.

tip: put this binary to your remote hosts should be sufficient as the one on localhost is not affected

https://github.com/SteveLauC/topgrade/releases/tag/oh-my-zsh-remote

Unfortunately, I will not be able to test it, since my remote is ARM.

@SteveLauC
Copy link
Member Author

Unfortunately, I will not be able to test it, since my remote is ARM.

Sorry, I don't have an arm-based machine at hand:(

@ahuston-0
Copy link

ahuston-0 commented Jul 14, 2023

Give me a bit and I can get back, my topgrade setup is all x86 stuff

@ahuston-0
Copy link

Hey sorry for the late response, but yes this does in fact work on my system
image
image

@SteveLauC SteveLauC merged commit 9415d7c into topgrade-rs:master Jul 18, 2023
8 of 10 checks passed
@SteveLauC SteveLauC deleted the remote branch July 18, 2023 06:00
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.

New "fatal" error with oh-my-zsh
4 participants