jorgebucaran / fisher

A plugin manager for Fish
https://git.io/fisher
MIT License
7.53k stars 257 forks source link

Fisher initial install command overwrites `fish_plugins` #705

Closed winghouchan closed 2 years ago

winghouchan commented 2 years ago

I have a fish_plugins file committed to my dotfiles so that I can quickly install Fish plugins on new machines. My config.fish also detects the presence of the fisher command and installs Fisher if it's not present. The current installation instructions specifies the following command to run: https://github.com/jorgebucaran/fisher/blob/93dafd242b52a0dc6bea54130d0ea041830c7fd6/README.md?plain=1#L17

The issue is the above command seems to overwrite whatever is in the fish_plugins file which means any other plugins in fish_plugins get removed. I'm working round this by doing the following instead:

curl -sL https://git.io/fisher | source && fisher update

Could the README be updated with the above recipe for installing Fisher if fish_plugins exists. Or could the fisher function be updated to not have this behaviour?

winghouchan commented 2 years ago

I took a look at fisher.fish and I think this is happening because:

  1. $_fisher_plugins is initially empty while the fish_plugins file is not.
  2. The installed plugin gets appended to $_fisher_plugins: https://github.com/jorgebucaran/fisher/blob/93dafd242b52a0dc6bea54130d0ea041830c7fd6/functions/fisher.fish#L170
  3. $_fisher_plugins then gets written to fish_plugins file: https://github.com/jorgebucaran/fisher/blob/93dafd242b52a0dc6bea54130d0ea041830c7fd6/functions/fisher.fish#L187
winghouchan commented 2 years ago

Hmm, #611 is kinda related to this. Syncing my fish_variables will ensure $_fisher_plugins is populated on initial install however syncing fish_variables isn't possible because it contains absolute paths which may break on different machines. If #611 is resolved in a way that allows fish_variables to be synced then this issue will be resolved too.

jorgebucaran commented 2 years ago
fisher install foo/bar

will (attempt to) install foo/bar and remove any plugins in your fish_plugins that are not already installed. This happens whether you are bootstrapping Fisher or not. I made it that way so that fish_plugins would always reflect the current state.

Try adding a bunch of words to fish_plugins, then fisher install jorgebucaran/foobar. Open your fish_plugins.

Anyhow, it seems that by using fisher update you were able to work around the issue.

Could the README be updated with the above recipe for installing Fisher if fish_plugins exists. Or could the fisher function be updated to not have this behaviour?

You are welcome to send me a PR adding the text you think is missing from the README, but I'm not too keen on changing how Fisher works here at this time.

winghouchan commented 2 years ago

I made it that way so that fish_plugins would always reflect the current state.

I see. I was making the assumption that fish_plugins would act like a plugin manifest and could be used to install plugins that weren't already installed given the following is in the README: https://github.com/jorgebucaran/fisher/blob/0a0c48993ae2bcf832fecff6efb0b11482183b6f/README.md?plain=1#L89-L91

And also making the assumption that fisher install would behave like the install commands of other package managers which don't modify the manifest at all.

But thanks for clarifying expectations here.

The README does mention the following: https://github.com/jorgebucaran/fisher/blob/0a0c48993ae2bcf832fecff6efb0b11482183b6f/README.md?plain=1#L99-L111

This is what prompted me to try fisher update instead of the command specified in the installation instructions.

If the README was to be updated, I'd do something like this:

## Installation

`` `console
curl -sL https://git.io/fisher | source && fisher install jorgebucaran/fisher
`` `

+ Installing a previous configuration with plugins? Use `curl -sL https://git.io/fisher | source && fisher update`.

What do you think? I'll open a PR if you think it's good.

jorgebucaran commented 2 years ago

...would act like a plugin manifest and could be used to install plugins that weren't already installed

Without arguments fisher update installs plugins not already installed, removes (already installed) plugins that are not present in fish_plugins, and updates everything else. Does this not act as a plugin manifest?

What do you think?

I'll definitely think about it. 😉

winghouchan commented 2 years ago

Well, not entirely since the typical expectation is a package manager's install command installs things on the manifest that aren't installed instead of treating the contents as a representation of the state of the system – which is more like a lockfile.

Ultimately, I've found a workaround, but it would be great for others to be able to find the workaround more easily hence the suggested addition in the README. Or for the behaviour of the install command to behave more intuitively. I'll leave it with you 🙂

jorgebucaran commented 2 years ago

I see! Perhaps we should discuss changing fisher install's default behavior without arguments. Right now it doesn't do anything. Instead, we could go ahead and install new plugins in fish_plugins and perhaps re-add removed entries of already installed plugins. I'm not sure about that last one.

winghouchan commented 1 year ago

Perhaps we should discuss changing fisher install's default behavior without arguments. Right now it doesn't do anything. Instead, we could go ahead and install new plugins in fish_plugins [...].

I think this is a sensible middle ground.

[...] and perhaps re-add removed entries of already installed plugins. I'm not sure about that last one.

I'm not sure either. Ultimately, I think it's a question of what is the source of truth for what packages should be installed. My view is, if fisher_plugins is supposed to be a package manifest, then it should be the source of truth for what packages are installed. This means:

LinuxIsCool commented 1 year ago

Whether I try fisher update or fisher install both commands are removing lines from fish_plugins in very unexpected ways. This makes it impossible for me to user fisher as a version controlled fish plugin manager and I will be seeking alternative options.

LinuxIsCool commented 1 year ago

This kind of works:
curl -sL https://git.io/fisher | source && fisher install (cat ~/.config/fish/fish_plugins)

But if any of the plugins fail to install then fisher removes them from the fish_plugins file which is very annoying.

jorgebucaran commented 1 year ago

Fisher's install command could add or remove lines from fish_plugins if you manually added or removed entries and then run fisher install. You probably meant to use fisher update instead, which works as I described here.

Perhaps we shouldn't let you run fisher install after you've modified the file, as that is probably a mistake anyway.

If the documentation is not clear about this, I'd love to fix it.

@LinuxIsCool Can you show one or more examples of either command removing lines from fish_plugins in unexpected ways?

LinuxIsCool commented 1 year ago

Thank you for the reply. fisher update was not working for me as you described here.

I got it working with the line of code I posted above. I don't have time right now, but if I get a chance, I'll see if I can re-create the situation I was in and post examples of what was happening when I tried fisher update.

jorgebucaran commented 1 year ago

Btw, you could fisher install < ~/.config/fish/fish_plugins as well. Sorry, I just have low tolerance for useless use of cat. 😂