wbthomason / packer.nvim

A use-package inspired plugin manager for Neovim. Uses native packages, supports Luarocks dependencies, written in Lua, allows for expressive config
MIT License
7.86k stars 267 forks source link

Allow a custom patch applied to plugins #882

Open lmburns opened 2 years ago

lmburns commented 2 years ago

Describe the feature

zinit (a zsh plugin manager) has a flag which allows the user to specify a file that contains a patch that gets applied to the repository after it is cloned.

I'm requesting a feature that would act in the same fashion, but for Neovim plugins.

danielnehrig commented 2 years ago

why not forking and point your plugin url to your fork url

lmburns commented 2 years ago

I mean that would work, but it seems like a bit much if all one wanted to do was change a single line. I know it doesn't matter a ton, but I'd prefer not to pollute the number of repositories I have if it isn't necessary, and doing a simple git diff -p > <plugin>.patch would make it a little easier. For instance, with zinit I have 26 different repositories I apply patches to. That's a lot of forks. Granted, there isn't as many repositories with Neovim that I feel the need to modify as there are with zsh.

Another reason is because (at least with zinit) the repository is kept up to date with the master branch. When updating other plugins, this repo would also update. The only difference is that a patch would be applied afterwards. If the patch failed to apply, a notification would be sent to the user. If this repository was a fork, you'd have to manually go in and pull from upstream.

I'll look into creating a pull request for this repository, or I'll figure out a way to do it with the set_handler method if this feature is undesirable

lmburns commented 2 years ago

Something else that zinit offers is to allow the user to set hooks both before and after cloning the repository, which I think would be useful. I'm not sure how much people use the set_handler feature, because I can only find ~30 or so results when searching Github for it.

lmburns commented 2 years ago

I have come up with the following, which does exactly what I want to do.

packer.set_handler(
    "patch",
    function(plugins, plugin, value)
        vim.validate {
            value = {value, {"b", "s"}}
        }

        if type(value) == "string" then
            value = fn.expand(value)
        else
            local plug_name = vim.split(plugin.name, "/")[2]
            value = ("%s/%s.patch"):format(PATCH_DIR, plug_name)
        end

        plugin.run = function()
            if uv.fs_stat(value) then
                api.nvim_echo({{"Applying patch", "WarningMsg"}}, false, {})
                cmd("lcd " .. plugin.install_path)
                Job:new(
                    {
                        command = "patch",
                        args = {"-s", "-N", "-p1", "-i", value}
                    }
                ):start()
            else
                api.nvim_echo({{"Patch file does not exist", "ErrorMsg"}}, false, {})
            end
        end
    end
)

I think that it would be nice to have this builtin. Though, I will close this issue

naquad commented 1 year ago

I suggest raising this feature request again. I'm providing a somewhat wordy explanation of why that feature or at least support facilities for it are needed from the user's perspective.

Rationale

NeoVIM has a voluminous and versatile plugin ecosystem utilizing a variety of languages (VimScript, Python, Lua, ...) and implemented for different versions of NeoVIM (VIM). Some plugins are out of date, some are abandoned, and some are in active development, ... Brownian motion at its best. NeoVIM is known for its flexibility and its users are building their own configurations to suit their needs by utilizing a set of plugins (potentially of a particular version) of their own choice and configuring them to play nicely with each other.

Potential plugin (and version) combinations are extremely huge. NeoVIM is not an IDE and has no facilities or standards to indicate that a particular functionality should be used/interacted with only in a specific way. This causes plugin conflicts and issues as some plugins are trying to modify or improperly interact with objects owned (created & managed) by other plugins resulting in errors, broken functionality, UX issues, and so forth.

It is up to the user to bring the Integration into their own setup. Plugins' configurational means are not always enough to achieve the desired integrity between various pieces. Code modifications may be required.

So the user may

  1. Request the plugin (or even plugins) maintainers to address a particular issue.
  2. Address the issue in the plugin(s) by themselves and send a pull request.

These two points are widely considered "the right way" (TM). In a perfect world, this would work 100% of the time and wouldn't cause any issues. The reality is different. Plugin's maintainers may neglect or reject the request based on their own vision of the plugin functionality & use cases supported or any other reason one can make up. Also, the plugin may simply be abandoned. Using abandoned/deprecated software is a subject of a separate discussion.

When "the right way" (TM) didn't work, the user can make a decision to:

  1. Fork the plugin, introduce the changes and switch to using it instead of the original one.

It is also considered one of "the right ways" (TM), such an idea has already been introduced above. But it has a major flaw: it introduces a technical debt to your editor(!). A tool. Technical debt in tools. There's a high probability that user's changes will break the upstream compatibility and the user won't be able to adopt improvements & futures developed in the upstream which makes the user a maintainer of the fork. Technical debt may sound too loud for this case, but as the person who spent two weeks migrating 10 y.o. VIM config to Lua trying to achieve the same workflow for 2 weeks, I assure you tech debt it is.

So the user got from "configure X, Y, and Z to play together nicely" to "maintain own fork of X". Users usually have much more than one plugin, so potentially it'll be "own fork of X, Y, Z, ...". Maintaining a fork requires effort on every editor and plugin update making sure the functionality (at least the utilized one) matches the upstream. 5-10 forks of actively developed plugins and the user gets a second full-time job.

One may argue, that I'm exaggerating the problem and the user would simply abandon the tries and replace a particular plugin or simply change a particular workflow. Well, that could be an option of course, but what's the point of high flexibility then?

So point 3 is off the table for purely practical reasons. Users won't be forking a bunch of plugins just to achieve a particular workflow.

What's left? The dark forest of hacks and workarounds.

  1. Monkey-patching

Overriding functions, objects, commands, adding extra handling with own autocommands (mess like au WinEnter * let w:current_focus = 1 that's later handled somewhere else...). An extremely fragile technique that will silently fail and potentially cause wreak and havoc. Sometimes it is an option, especially when the plugin is not actively developed. The complexity of this approach is that not everything can be overridden or patched. Especially this relates to Lua's local declaration. getupvalue and setupvalue are inefficient and cause things to blow in the most unexpected places, overriding whole modules (package.loaded['...'] = require('my_own_hacked_module')) still may introduce issues as it won't be compatible with the new version. And you can't know before you invoke the functionality utilizing the code, maybe even in a very subtle way.

Could be an option, but it is too fragile and fails silently. What is more assertive?

  1. Patches!

They are around almost since the beginning of time. Every distro has some set of patches for the kernel, packages, or configurations, quite a few other software solutions incorporate patches for their dependencies into the build process. It is a common thing. Well, for the customization of the 3rd party code at least. Or even binary data. Patches are much more reliable than monkey-patching in the runtime as the patching process is assertive, so the patch can't be applied an error will be raised. There still may be breaking changes in other files but patching covers at least some potential errors, unlike the above hacks and workarounds. Also, they're much simpler to write as one doesn't need to think how to properly override in the runtime a particular function or a piece of code as it's simply a code that will be edited. No reflections, no digging in the exposed structures or the guts of the interpreter. Pure code edit.

One may argue, that users will end up with a ton of patches that they will have to maintain. Well, any kind of good idea can be abused. The patch itself should be small, it can be quickly withdrawn from the setup (build?) process, and later the user can come back to it. A much lower effort than forking the upstream and setting up automated merges of the mainstream changes.

Adding at least some facilities to the package manager will make patching easy and reliable.

Potential solutions

I've spent some (minor) time trying to integrate my patch application with the packer.nvim and found it to be not as easy as expected. I blame my lack of packer.nvim internal structure knowledge and the absence of time to dive deeply into it, so my discoveries in the first approach may be wrong, please correct me if that's the case.

There are two potential solutions:

Patching support: a complete solution

A complete solution would imply providing the following commands:

PatchRevert should be invoked before the plugin updates to don't break the update process.

PatchApply should be invoked after the plugin has been installed or updated.

This is a full-blown solution that should cover 99.9% of all patching needs. It is quite a lot of work and maybe it should be implemented by some other plugin interacting with packer.nvim rather than in packer.nvim itself. So

Patching support: minimal facilities

Packer.nvim is a package manager after all and it is good at what it does, so it shouldn't try to do everything at once. Fair enough. While evaluating the way to implement patching support in a separate plugin I've discovered that packer.nvim is missing pre-installation/update hook and global hooks. While the latter is not a big problem (one can wrap use function and inject settings in the wrapper), the former one is a problem. Before updating the plugin one needs to revert the patches to avoid breaking the merge of upstream changes. It is ok if the patch fails to apply, it is not ok if the working tree becomes invalid or gets extra commits (for merges).

So there are two updates:

  1. Introduce a pre-install/update hook
  2. (Optional) introduce global hooks specification

Then it will be easy even for the user to implement their own patching functionality, not speaking of the 3rd party plugins capable to provide much more sophisticated facilities described above.

lmburns commented 1 year ago

Sorry for such a late reply, I had it written down to get back to but just never did. I really like your explanation for this, so I will go ahead and reopen this. Plus having the commands that you have mentioned would help quite a lot as well. I'm not sure what would actually get implemented, but I would imagine it would be better than what I have written as a handler above.