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: zinit default install location #625

Merged
merged 1 commit into from
Feb 17, 2024
Merged

Conversation

SteveLauC
Copy link
Member

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

For new steps

  • Optional: Topgrade skips this step where needed
  • Optional: The --dry-run option works with this step
  • Optional: The --yes option works with this step if it is supported by
    the underlying command

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.

From zinit's README, it seems that they have changed their default install location from ~/.zinit to ~/.local/share/zinit, making our detection fail, this PR fixes it.

@SteveLauC
Copy link
Member Author

cc @karman011, here is a build for x86_64-unknown-linux-musl, welcome to give it a test

@karman011
Copy link

karman011 commented Dec 4, 2023

@SteveLauC
image
I think parallel update caused the error.
Anyway now it can detect zinit on my machine. But ZINIT[HOME_DIR] can be set by user. Can we just check whether ZINIT[HOME_DIR] is set or not?

@SteveLauC
Copy link
Member Author

I think parallel update caused the error.

Friendly ping the author of #614 @domglusk, could you please give it a look?


But ZINIT[HOME_DIR] can be set by user. Can we just check whether ZINIT[HOME_DIR] is set or not?

Seems we should do this, but I am curious what is the difference between ZINIT_HOME and ZINIT[HOME_DIR]

ZINIT[HOME_DIR]: Where Zinit should create all working directories, e.g.: "~/.local/share/zinit"

From their README, it seems that they are the same thing

@SteveLauC
Copy link
Member Author

Oh, I am sorry, they are different things:

By default, ZINIT_HOME will be set to ~/.local/share/zinit/zinit.git, and ZINIT[HOME_DIR] will have value ~/.local/share/zinit


I kinda think we don't need to check ZINIT[HOME_DIR] now, the check here is simply to figure out whether zinit is installed by the user, if the user chooses to use the automatic script, then file ~/.local/share/zinit will be created, if manual installation is chosen, then env var ZINIT_HOME will be set in the .zshrc, which will also be detected by Topgrade, our code will work in either way

@SteveLauC
Copy link
Member Author

Well, I tested the command topgrade uses to update zinit, sourcing .zshrc would return 1 and thus fails the step:

[root@ef5a35a4063d]~# ./topgrade --only shell

── 09:27:56 - zinit ────────────────────────────────────────────────────────────
zinit failed:
   0: Command failed: `/usr/bin/zsh -i -c 'source /root/.zshrc && zinit self-update && zinit update --all -p'`
   1: `/usr/bin/zsh` failed: exit status: 1

Location:
   src/steps/zsh.rs:134
Retry? (y)es/(N)o/(s)hell/(q)uit

── 09:27:57 - Summary ──────────────────────────────────────────────────────────
zinit: FAILED
[root@ef5a35a4063d]~#

But executing zinit self-update && zinit update --all -p seems to be fine:

[root@ef5a35a4063d]~# zinit self-update && zinit update --all -p
[self-update] fetching latest changes from main branch
From https://github.com/zdharma-continuum/zinit
 * branch              main       -> FETCH_HEAD
Already up to date.
[self-update] compiling zinit via zcompile
[self-update] reloading zinit for the current session
[self-update] updating zinit repository
[self-update] fetching latest changes from main branch
Parallel Update Starts Now…
The update took 3.68 seconds
[root@ef5a35a4063d]~# echo $?
0

@SteveLauC
Copy link
Member Author

I doubt it runs without any issues because I didn't have any plugin installed, then I installed a plugin with zinit load zdharma-continuum/history-search-multi-word and ran that update command again, still, I got no issues.

@SteveLauC
Copy link
Member Author

Let's merge this

@SteveLauC SteveLauC merged commit 7c63541 into topgrade-rs:main Feb 17, 2024
7 checks passed
@SteveLauC SteveLauC deleted the zinit branch February 17, 2024 05:15
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.

2 participants