jdugan6240 / kak-bundle

Mirror to https://codeberg.org/jdugan6240/kak-bundle. Contributions are accepted here, but they are preferred upstream.
BSD Zero Clause License
30 stars 3 forks source link

Kak-Bundle Design Discussion #2

Closed mralusw closed 1 year ago

mralusw commented 3 years ago

It seems bundle is adding a %val{runtime}/autoload symlink to bundle's dir, which is in turn under user's autoload — this will result in nasty, hard to debug errors in most setups, as people usually already have that symlink directly under their autoload.

So you should decide whether you want bundle to

Also, there are also shell quoting problems (use "$var" consistently, unless you really mean to word-split, but beware that a naked $var also expands globs), and perhaps bundle_plugins should be a str-list option.

jdugan6240 commented 3 years ago

As for your overall point, although I agree about the symlink thing (I could include an option to do it or not), I think the rest of that first bullet point betrays a misunderstanding of the purpose of the bundle command.

We are using autoload precisely so that we don't have to handle the actual loading of plugins, which makes this significantly faster on load than the OG plug.kak. This was also the aim of alexherbo2's plug.kak. What the bundle command actually does is tell kak-bundle which plugins to install when the bundle-install command is run. Admittedly, I probably didn't make that very clear in the README.

You do have a point about the shell variable expansion and using a str-list instead of a space delimited string for the list of plugins, though - I will certainly look into that.

mralusw commented 3 years ago

OK, so you want bundle to stay in autoload (and not call find), that's good to clear up.

In this case, to remove a plugin, the user not only has to remove the corresponding bundle command in their kakrc, but also manually remove the git repo from under bundle/. This needs to be mentioned in the README.

There's another option that doesn't require find: move the actual git repos out of autoload (e.g. into %val{config}/bundle-repos), and add symlinks to the "enabled" plugins (actual git repos) under a "thin" directory autoload/bundle. Or some other default names. It's more complex, but you get to manage what gets loaded (via the bundle command) without slowing down to find / source.

If you like this last option, I can help with that (it sounds like something I might want to use). If you prefer to keep it as it is, that's also great, but I'd suggest not adding the runtime/autoload symlink by default, as it breaks most existing setups. I've struggled to debug a stray duplicate runtime/autoload link for a day recently (added by my own testing scripts for a plugin, long story).

jdugan6240 commented 3 years ago

I really like the way you're going with this, but I have another option that would avoid having to deal with symlinks: add a bundle-load command that takes a set of plugins as parameters. For example, if you wanted to load mru-files.kak (great plugin, by the way) and kak-lsp, you would run bundle-load mru-files.kak kak-lsp. This command would go into the bundle plugins directory and load all the .kak files found in the repos of the specified plugins. This would allow the user to pick and choose what plugins to load, and I don't have to deal with cleaning up symlinks to plugins the user no longer wants loaded in a future Kakoune session. This could look like the following in someone's kakrc:

bundle https://github.com/kak-lsp/kak-lsp
bundle https://gitlab.com/kstr0k/mru-files.kak
bundle https://github.com/andreyorst/powerline.kak

bundle-load kak-lsp mru-files.kak # Note that only kak-lsp and mru-files get loaded, not powerline.kak

# Now plugins are loaded. Set options and hooks and run commands here.

The main slowdown with the OG plug.kak that I wanted to avoid is actually the fact that each invocation of the plug command spawns an sh process, with all the os-level processing that entails. The main goal with this plugin manager is to avoid creating sh processes as much as possible, not necessarily to avoid calling find (Kakoune actually uses find to load the autoload directory - see https://github.com/mawww/kakoune/blob/master/share/kak/kakrc) - hence why the bundle command does no more than simply update the plugin list. Placing all the plugins in autoload was my initial plan on accomplishing this goal, as Kakoune would spawn a single sh process to load them all. However, loading the plugins with a bundle-load command would require creating only one sh process on my part, and allows the user to populate their autoload directory as they see fit, avoiding the symlink collision issues you mentioned earlier.

I hope I don't come across as too callous regarding your input - I really do appreciate your willingness to give input and get involved in making this plugin manager work.

mralusw commented 3 years ago

Thanks, and no problem at all. Hey, we're on the same page — as you may have noticed I've also been working on reducing %sh{} calls (or if not possible, at least the number of subshells / external calls in shell code).

bundle-load sounds like a good design.

Another thing: for some git repos, I need to switch to a non-default branch (or a tag). I'm OK with doing that manually, once after bundle-install, but if I clean and re-install, the branches / tags get lost; it gets unworkable. What do you think about this? One way I can think of to handle it would be to optionally provide the bundle command with a full git command (bundle 'git clone -b BRANCH ... $URL'. Then bundle-install could tell if each plugin (from the str-list) is a simple URL or a full git command ending in a URL (it has spaces); bundle-update doesn't care. This would be backwards compatible.

jdugan6240 commented 3 years ago

Sorry for the delay - I was busy all morning. I've taken the liberty of renaming this issue to "Kak-Bundle Design Discussion", as that is essentially what this has turned into (not that that's a bad thing).

I really like that idea of potentially passing a git command to the bundle command as a means of specifying a certain branch or tag to switch to. Essentially, it would just require the bundle-install command checking if the string starts with 'git ', and deciding what git command to run based on that. I don't really have time to work on that right now, but I think that's a good way to move forward.

jdugan6240 commented 3 years ago

The bundle-load command has been added, and it works exactly like we discussed. It seems to work for me, so let me know if you have any issues with it. I don't have time to implement the bundle command enhancement tonight, though.

mralusw commented 3 years ago

Let me look at it and maybe PR some changes. Some quick remarks:

I'll implement the bundle 'command args... URL' extension also. A further enhancement:

mralusw commented 3 years ago

Note: with bundle 'git clone URL ./renamed', the user can also handle multiple repos with the same name (but different URLs).

jdugan6240 commented 3 years ago

OK, I rewrote the README to hopefully better reflect all the new things kak-bundle is now capable of. If anything was omitted or is unclear, don't hesitate to let me know.

mralusw commented 3 years ago

Will do, I'm a little busy with sel-editor right now.

But since I'm here, I was thinking about a few more things (briefly explained, let me know if anything sounds confusing):

There are some undocumented capabilities; maybe we should document, maybe sit on them:

mralusw commented 3 years ago

I.e. this simply means sending back a list of

set -add global bundle_new %<source %<XXX>;>
set -add global bundle_new %<source %<YYY>;>

instead of the plain source commands. Since <> are already excluded from filenames, and since they balance, it makes no difference (otherwise we'd have to add a quoter, quote-quote the filenames etc)

In Kakoune, after eval -- %sh{},

# set-difference: don't load again
set -remove global bundle_new %opt{bundle_loaded}
# "%opt{}" concatenates "source" statements with spaces between
eval "%opt{bundle_new}"

# remove, then re-add to get set-union
set -remove global bundle_loaded %opt{bundle_new} 
set -add global bundle_loaded %opt{bundle_new}

Other solutions are possible (like a command_fifo to have sh compute some things etc)

mralusw commented 3 years ago

So, OK, this worked quite nicely it seems (PR #5 ).

Now, I'd like to write a bundle-plug emulator, which I would release separately; it would help adoption, and also keep kakrc configs more-or-less compatible. It would provide a bundle-plug command, which people can optionally alias to ... plug if they wish to keep their configs.

I would need a command that both registers the plugin and calls bundle-load with the final /* component (basename), for all args. This approach would be slower, but the tradeoff might be worth it to some. This should be quite easy. But what do you think?

Note that I've already implemented plug-many for plug.kak, which loads multiple plugins with a single sh call (see the PR's there). It's fate is not quite decided (apparently there will be a new plug version at some point anyway), but it works.

jdugan6240 commented 3 years ago

So, something like this, perhaps (sorry if this is suboptimal; my bash-fu is weak)?

def bundle-plug -params 1 %{
    bundle %arg{1}
    eval %sh{
        plugin="$1"
        repo="${plugin#*/}"
        echo "bundle-load $repo"
    }
}
alias global plug bundle-plug

Obviously that example would need some tweaks, as it only works with passing a URL to it, and it doesn't support any of plug's switches, but I agree that including something like this would make migrating from plug.kak to kak-bundle substantially easier.

mralusw commented 3 years ago

Yeah, except more like https://github.com/andreyorst/plug.kak/pull/100 :

bundle-plug https://github.com/gustavo-hms/peneira demand peneira %{
} plug https://github.com/jbomanson/search-doc.kak demand search-doc %{
  alias global doc-search search-doc
} plug https://gitlab.com/FlyingWombat/case.kak

... so that there can be a single command and sh call.

From plug, I would take config, demand and defer (make config mandatory — the goal is to make compatibility possible and not split the community; not to emulate perfectly).

So, I would probably need something like

bundle-load-and-register %{ prelude1 } URL1 %{ postlude1 } \
 %{ prelude2 } 'cmd .. ./myplug' %{ postlude2 } \
 %{ prelude3 } URL3 {postlude3 } # etc
## or maybe URL prelude postlude

... from bundle. As a first (and probably uncontroversial) step. Then I could implement a simple plug emulator. In kak, first bundle-source is executed, then the postlude is eval'd. The post-lude (to be generated by bundle-plug) would be: plug's config + register a ModuleLoaded hook + require-module # if demanded

The preludes could be sh code. I'm not sure yet. I just put them as placeholders for now.

There are other designs (as in that plug PR); for instance

bundle-block-start
bundle-block-one %{ prelude1 } URL-or-cmd1 %{ postlude1 }
bundle-block-one %{ prelude2 } URL-or-cmd2 %{ postlude2 }
bundle-block-end

This could accumulate args in a str-list, and (bundle-block-end) finally send them to %sh{}.

What do you think (as far as architecture, naming etc)?

jdugan6240 commented 3 years ago

OK, let me see if I understand you. If we were to go with your first suggestion (which is the one I'm leaning towards, in all honesty):

bundle-load-and-register %{ prelude1 } URL1 %{ postlude1 } \
%{ prelude2 } 'cmd .. ./myplug' %{ postlude2 } \ 
%{ prelude3 } URL3 {postlude3 } # etc

Then this hypothetical bundle-load-and-register command would need to do the following (I'm ignoring the preludes for now, because I don't think we need them):

If that is correct, then this should be doable. Again, I'm no shell shogun, but I should be able to cobble up something that does the above.

mralusw commented 3 years ago

PR #6 . Actually, it was a little tricky to get right. To avoid adding a quoter, I'm passing back kakoune %arg{} references that only make sense to the calling function. But, this is %sh{} code that is supposed to be bound to a kak command anyway.

Feel free to amend as you see fit / let me know if you want to change it. I've called it *-register-and-load, since that's the actual order. It also needs docstrings etc.

BTW, do you have any local tests? Are you using bundle with a reasonably complex config yourself? Mine includes only a converted portion of the plug configs, but this will change as soon as I write the bundle-plug emulator.

mralusw commented 3 years ago

... and, at some point, we should probably split the prelude into a separate *.sh file. Let me know when you think it's a good time (since again, this breaks git diff viewability)

jdugan6240 commented 3 years ago

You're too quick, dude. Leave me something to do 😄

Also, I'm in the same boat as you - my config effectively amounts to the following:

bundle https://github.com/johndoe/blah_blah_blah
bundle https://github.com/janedoe/blah_blah_blah

bundle-load # Load plugins blah_blah_blah and blah_blah_blah

Not exactly the most exciting thing in the world for testing purposes.

mralusw commented 3 years ago

Sorry about that... I realized it might be a problem. I happen to have a quite a bit of "OSS time" at this point, I guess, and I also want to get things "just right".

I really appreciate your design decisions / input (as I've said, without it I might have over-engineered things even worse). So, what do you see next in bundle / what would you like to implement? Just so we don't step on each other's toes, I'm planning to do this:

jdugan6240 commented 3 years ago

Oh, no worries. That was meant in jest 😉. It's just that, during weekdays, I have maybe an hour or two at most to work on side projects, due to my job and other priorities, and I have kak-dap to work on as well.

As for your suggestion to work on viewing the install/update process in a buffer, that was going to be my next step. I don't think we need a fancy interface like the OG plug.kak for this - outputting the stdout of the git commands (or whatever we're running) into the buffer should be enough. We could even give it the grep filetype for some highlighting. I believe that's how alexherbo2's plug.kak did it, before he discontinued it. I'm not even entirely convinced the parallel processing will make this that much harder, although the option still exists to nix the buffer if $kak_opt_bundle_parallel is set to true, should things get dicy.

I was also planning on a command to pick and choose which files to load within a plugin. For example, lenormf's kakoune-extra repo is just a collection of scripts, and chances are the user doesn't want them all. Maybe a bundle-pickyload command could accept a plugin name and some scripts as arguments, and we would just source those scripts. There may even be a way to get this to work with multiple plugins...

jdugan6240 commented 3 years ago

Hang on - I have a better idea for the bundle-pickyload command. Instead of passing the plugin name and scripts separately, define them in the same argument. Something like this:

bundle-pickyload kakoune-extra/autosplit.kak kakoune-extra/fzf.kak

This also makes supporting multiple plugins with this command trivial, as basically all we're doing is cd'ing into the plugins directory and sourcing each of the arguments.

mralusw commented 3 years ago

Hey, kak-dap looks like a really cool project.

pickyload also seems like a worthy extension. What do you think about adding a filter option bundle_source_filter, which the user can manipulate at their convenience, to... filter out (or filter-in) sources? This would allow all -load-related commands (register-and-load, bundle-plug) to implicitly use it. Then, bundle-pickyload itself could be a wrapper.

mralusw commented 3 years ago
bundle-pickyload kakoune-extra/autosplit.kak kakoune-extra/fzf.kak

This also makes supporting multiple plugins with this command trivial, as basically all we're doing is cd'ing into the plugins directory and sourcing each of the arguments.

Ah. I see what you're saying, that would be much simpler. But then how would you exclude those picky-ed plugins from the general bundle-load # no args at the end of kakrc?

jdugan6240 commented 3 years ago

I would probably solve that by having bundle-pickyload update a list of plugins it's handled, so bundle-load knows not to touch them.

mralusw commented 3 years ago

I would probably solve that by having bundle-pickyload update a list of plugins it's handled, so bundle-load knows not to touch them.

I see, so something like

printf '%s\n' \
  "set -remove global bundle_loaded_sources %<${file%%/*}>"
  "set -add global bundle_loaded_sources %<${file%%/*}>"

This is a really nice enhancement.

mralusw commented 3 years ago

https://github.com/kstr0k/kak-bundle-plug

It works with my entire setup. Pure kakscript, for better and worse.

mralusw commented 3 years ago

As for your suggestion to work on viewing the install/update process in a buffer, that was going to be my next step. I don't think we need a fancy interface like the OG plug.kak for this - outputting the stdout of the git commands (or whatever we're running) into the buffer should be enough.

The problem with multiple jobs is that if all jobs output to the same stdout, their output gets mixed up. Even producing mixed-up line fragments from different jobs.

The cleanest output can be obtained by redirecting the stdout of each job to a separate log (which is what I did). Now, to get that output, either the shell has to pipe back to kak (FIFO or command-fifo), or kak has to read these logs.

If the shell is to concatenate the logs (either by looping over them once a second, or with tail -f file1 file2 etc, which is not even POSIX for multiple files), then it also has to wait for all jobs to terminate. So it has to wait for all jobs except the one that polls the file. This is tricky. The POSIX wait command doesn't take a timeout. There's kill -0 to check the existence of a PID, but I'm still not sure it's completely safe. It may be possible to wait for all jobs to finish in a job-owner subshell, and in the main process poll (with kill -0) that job-owner, while concatenating the logs on each iteration and sending them to kak.

If, OTOH, Kakoune is to collect the logs, then the shell can still wait for all jobs to finish as it's doing now. However we need to implement a periodic hook in Kakoune, e.g. by chaining -once NormalIdle hooks and increasing a counter which wraps around to 0 when the periodic limit is hit (at which point the periodic code executes). This is also tricky, especially since the NormalIdle timeout is not fixed (from what I recall). And NormalIdle doesn't occur without user activity...

jdugan6240 commented 3 years ago

OK, bundle-lazyload is in the latest master. It not only selects specific scripts to load from a plugin, but it also marks that plugin as "loaded" so future bundle-load calls don't try to load the rest of the plugin. From my testing, this seems to work fairly well. I also removed the bundle_new_sources and bundle_loaded_sources options, since these seemed to me to be overcomplicating things, and a single bundle_loaded_plugins option seemed to do the trick. Let me know if this command works for you.

I think I'm going to need to take a break from kak-bundle for a bit - I only have limited time right now and need to get kak-dap done at some point.

mralusw commented 3 years ago

Well first of all there is no POSIX [[ .. ]] or =~ operator. Are you using bash as the KAKOUNE_POXIX_SHELL instead of dash?

mralusw commented 3 years ago

OK, I understand -- it's better to focus on the things you like.

But I'm not sure how to proceed if you want to take a break. The simplest option for me would be to develop my fork independently. I have yet to implement a number of things (e.g. noload support).

Another option would be for me to take over bundle for a while, until you want to work on it again, if you'd like to proceed that way.

What do you think?

jdugan6240 commented 3 years ago

I just pushed up a change that should address the POSIX compliance issue.

About how to proceed from here - I would say just work on your fork, and then I'll have you submit a pull request when I'm ready to get back in the saddle. This is basically what andreyorst did when he had to take a break from developing powerline.kak and I was maintaining it for him.

mralusw commented 3 years ago

OK;

  1. let me finish my kak-bundle-plug emulation
  2. including any PR's I may need to kak-bundle
  3. then let's make a joint announcement

And then you can take a break :) Does that sound reasonable?

jdugan6240 commented 3 years ago

Sounds good.

mralusw commented 3 years ago

Well, I've finally looked at your changes. For one thing, bundle doesn't run. You're using [ x == y ], which to my knowledge is not supported in any shell. It's either [[ x == y ]] in Bash, or [ x = y ] in POSIX. What shell are you using?

Then, you've decided to remove the "overly complicated" bundle_*_sources, so now you have to iterate (quadratically) over plugins in the shell, because of course the shell doesn't have mathematical sets (while kakoune's str-list's work as such). Not so much of a slowdown, but I'm not sure what the point is. Perhaps I should have explained more.

Finally, you've decided to make loaded_plugins a space-separated str instead of a str-list.

I've just created a fixes branch, I'll send you a PR.

Nice work on lazyloading, though.

mralusw commented 3 years ago

There are also several new external calls (ls, cut) with subshells to boot

jdugan6240 commented 3 years ago

Those are fair enough criticisms. I honestly don't know what I was thinking with those conditionals 😜.

I can't look at your pull request right now, but I'll get to it tonight.

jdugan6240 commented 3 years ago

Just merged your changes this morning. Thanks again for looking into that.

mralusw commented 3 years ago

OK, great, just make sure pickyload works, because I've made some ("obvious") changes in that part of the code without testing.

mralusw commented 3 years ago

Well, I think bundle is quite ready at this point. I am currently able to toggle back and forth between plug (with the recent plug-chain command) and kak-bundle-plug with a single alias command.

Some results comparing plug and kak-bundle, with regular dash as well as busybox sh (not much to brag about under busybox, since many UNIX commands are builtins in that shell; but a large improvement under dash) : Command Mean [ms] Min [ms] Max [ms] Relative
KAKOUNE_POSIX_SHELL=/bin/dash KAK_PLUG_CHAIN=plug-chain /opt/kak/bin/kak -ui dummy -e quit 282.1 ± 2.9 277.6 285.8 1.84 ± 0.04
KAKOUNE_POSIX_SHELL=./busybox-static+applet_bias-1.33 KAK_PLUG_CHAIN=plug-chain /opt/kak/bin/kak -ui dummy -e quit 153.1 ± 2.5 147.5 157.1 1.00
KAKOUNE_POSIX_SHELL=/bin/dash KAK_PLUG_CHAIN=kak-bundle-plug /opt/kak/bin/kak -ui dummy -e quit 244.1 ± 2.6 240.1 247.5 1.60 ± 0.03
KAKOUNE_POSIX_SHELL=./busybox-static+applet_bias-1.33 KAK_PLUG_CHAIN=kak-bundle-plug /opt/kak/bin/kak -ui dummy -e quit 158.5 ± 2.1 155.3 163.4 1.04 ± 0.02

The only thing that remains is documentation. Can you document pickyload? I could write a few lines about register-and-load, plugin URL->path resolution, as well as document kak-bundle-plug.

jdugan6240 commented 3 years ago

All right, will do.

Those performance statistics would also be good for the README, I think, under a Performance header to showcase the speed, since loading time was the main purpose for creating this plugin manager in the first place. How many plugins were you loading during those tests, out of curiosity?

jdugan6240 commented 3 years ago

OK, documentation on bundle-pickyload and the new bundle_git_shallow_opts option should now be up. Let me know if there's anything I missed.

mralusw commented 3 years ago

I think I could pretty much copy the bundle-register-and-load documentation from the https://github.com/kstr0k/kak-bundle-plug README (which I've just committed). How does it look to you?

Also, do you want to mention my add-on? I hate bragging about my own stuff in other people's backyard :) But I think it's really nice to be able to switch back and forth between the two plugin managers with a single command.

jdugan6240 commented 3 years ago

I'll be sure to mention kak-bundle-plug in the Tips and Tricks section, under a plug.kak Compatibility header or something. Having an easy way to transition from plug.kak to kak-bundle would be very appealing.

That said, for a while now I've been in the process of migrating my repositories to Codeberg, due to recent (as in the last couple years) business decisions on Github's part that I'm not really much of a fan of. I've already migrated kak-dap over, and this plugin is next in line. If you want to keep contributing to this plugin (and I'm assuming you do), then I believe Codeberg lets you log in with your Github account, so the barrier to entry shouldn't be too high. We can continue this discussion over there.

If you need me to delay for a bit, I can do that, but I would like to get moved over eventually.

mralusw commented 3 years ago

It depends on what you want to announce on http://discuss.kakoune.com; I've been saying we're nearing completion for a while, but this time I think it's for real :)

I presume Codeberg? I've managed to sign-up and link accounts (whatever that means — you still need to create a CB account in the first place).

I'm mostly on gitlab myself (https://gitlab.com/kstr0k).

Now: are you going to delete this GH repo? Abandon it? Mirror Codeberg?

mralusw commented 3 years ago

I've been mentioning the URL of your repo in several discussions, including ongoing bug reports for https://github.com/mawww/kakoune — it would be a pity if those links broke.

jdugan6240 commented 3 years ago

I wouldn't get rid of the github repo entirely - I would just replace the README with a statement that development has moved to Codeberg and archive the repo, like what I did with kak-dap: https://github.com/jdugan6240/kak-dap.

In other words, linking to it wouldn't yield a 404, but there wouldn't be any updates to that repo.

mralusw commented 3 years ago

I see. Well, copy it over to begin with, and let me see how it works.

jdugan6240 commented 3 years ago

OK, the new repo can be found here: https://codeberg.org/jdugan6240/kak-bundle.

This design discussion issue was even copied over, so should the need arise, we can continue in there.

jdugan6240 commented 1 year ago

Given the recent kak-bundle redesign, I think this issue can be closed.