Closed mralusw closed 3 years ago
OK, bundle_parallel
implemented too. It's off by default. TODO: if the user has a ton of plugins, limit the number of parallel jobs: execute batches of N
, wait, repeat.
Hmm. I'm not sure about this, in all honesty.
I agree emphatically with the idea of bundle-install
and bundle-update
happening in the background. The only reason I didn't do that to begin with was because I couldn't find an easy way to inform the user that the processing was done.
I would probably prefer to send the stdout of the shell processes run by bundle-install
and bundle-update
to a FIFO buffer, so the user can visually see the command's progress. We can send this process to the background as well.
Introducing the verbose and parallel options seems to be introducing a lot of unnecessary complexity, as well. Going back to what I said above, if we output the stdout of bundle-install
and bundle-update
to a FIFO buffer, we don't need a "verbose" option IMO. Also, since we would be running this process in the background, we aren't blocking anymore, which means parallel processing would have fairly limited benefit.
Finally, I'm not a big fan of having a giant chunk of sh code in bundle_sh_code
. It makes it difficult (for me at least) to decipher what exactly each call is doing, and makes the code hard to follow. I would personally prefer each sh function be defined within the bundle command that uses it, just for proximity's sake.
Sorry if I come across as harsh here - I greatly appreciate your enthusiasm and effort, and there's genuinely a lot to like here (I really like the git_clone_opts
option, for example).
Well, then merge upto wherever you agree with (which is?...), and we can have divergent branches for a while. The current setup suits me for now; but I can an eye on your developments.
Regarding verbose, don't you think the user should optionally be able to see (but most of the time not...) exactly what is being loaded / executed when installing?
That big chunk (bundle_sh_code
, I think?) is for factoring out correct code. For instance, you always want set -u
(otherwise a single mis-spelled variable can lead to a disastrous rm -Rf
), you always want to be in the correct directory (hence bundle_cd
), you always want the cleanup steps to be the same (rm
, mkdir
, ensure created) etc.
As for parallel, yes, it is more complex (and I disabled it by default). Without it, the slow sequential installs / updates, even if non-blocking in principle, keep me looking at the screen, so it's blocking psychologically.
Edit: actually, it's blocking in principle too, because you need to restart kak.
Actually, now that you've explained your rationale, all of your changes are making a lot more sense. I'm going to merge this once I get home from work.
Hey, your call — I also tried to keep it as simple as possible for as long as I could. OTOH when you do need complexity, I think it's best to factor it out and reuse it.
Would it help to cut-off this pull request at https://github.com/jdugan6240/kak-bundle/pull/4/commits/fa3aaf33cd31aa33736ad9f5b19d133e1aae264d and make a separate one for further changes?
I also agree that there should be some FIFO
. Perhaps using the new command_fifo
? I haven't tried it out yet. It seems better than a FIFO buffer in that kak manages the FIFO file; you can also send arbitrary commands, e.g. tell kak about where per-job temporary files live, have it poll job status in a periodic hook — I can see a lot of potential coolness, with more complexity, though :)
I would say that cutting this pull request at that point would be a good course of action. I'd like to be sure that we need this complexity before adding it :)
I'm not even opposed to the idea of refactoring out the shell code in a similar way to how you did - I guess just placing it in a Kakoune option kind of caught me off guard. Maybe something like how plug.kak separates the shell code from the kakscript?
I haven't looked into command_fifo
too deeply, but I'm not sure about it. It could be that I'm just misinterpreting the docs, but it seems to just be a pipe that executes the commands passed into it as soon as it's closed. The reason I want to use a FIFO is for the user to visually see the installation/update progress, and that can be done by creating a FIFO pipe and opening it in a buffer, like a lot of the builtin kakscripts such as grep and make. I don't see how command_fifo
accomplishes that. It's undoubtedly a cool feature, don't get me wrong, but it may not be what I'm looking for here.
I would say that cutting this pull request at that point would be a good course of action. I'd like to be sure that we need this complexity before adding it :)
OK, will do... Though, honestly, the fork will probably be more featureful than the original — unless you change your minimalist design goal.
The reason I want to use a FIFO is for the user to visually see the installation/update progress, and that can be done by creating a FIFO pipe and opening it in a buffer, like a lot of the builtin kakscripts such as grep and make. I don't see how
command_fifo
accomplishes that. It's undoubtedly a cool feature, don't get me wrong, but it may not be what I'm looking for here.
OK, try this (you'll need to save, then :source /tmp/kkff.kak
)
nop -- %sh{
kkq() {
while [ $# -gt 0 ]; do
printf " '"; printf '%s' "$1" | sed -e "s/'/''/g"; printf "'";
shift
done
} #quoter
kkff() { cat >"$kak_command_fifo"; }
echo 'edit -scratch *bundle-status*; exec %(%d)' | kkff
for i in $(seq 1 5); do
kkff <<EOF
reg dquote $(kkq "$(date)"); exec 'gg' '<a-x>"_d' '<a-O>h' 'P'
reg dquote $(kkq " '$i' ... "); exec 'geP'
EOF
sleep 1
done
}
Notice how the date and the log update separately. You can display multiple logs this way.
But, I'm sure you won't like the complexity :))
I'm not even opposed to the idea of refactoring out the shell code in a similar way to how you did - I guess just placing it in a Kakoune option kind of caught me off guard. Maybe something like how plug.kak separates the shell code from the kakscript?
I've thought about it, but that would have been an even bigger delta; I figured separation breaks git diff
viewability, and can be done whenever we decide. Plus, going back to your concern about being able to figure out what's going on, having to switch between two separate source files probably makes it even harder to follow.
I've arrived at placing code in options after a lot of tinkering in the past (as is the case with a lot of "unconventional" approaches). mru-files
tries to touch the disk as little as possible (I run live Linux distros from USB sticks; merely switching buffers shouldn't trigger disk activity). Therefore I load code from a .sh
file (on disk) into an option (in memory), and use it as a shell prelude.
All things said, for a small .kak
source like bundle
, keeping the prelude in an option seemed like the best choice when tinkering. Yeah, it's unconventional so it causes surprise.
OK, will do... Though, honestly, the fork will probably be more featureful than the original — unless you change your minimalist design goal.
Honestly, it's about time I admit that kak-bundle isn't minimalist anymore 🙂. It's grown to be something way beyond what I had initially envisioned, but that isn't necessarily a bad thing. I don't think being minimalist is going to be a design goal anymore, because I think we've passed the point of no return on that front.
OK, try this (you'll need to save, then :source /tmp/kkff.kak)
So that's how that works. Assuming that doesn't leave a residual FIFO file on the disk, that seems to be a fairly elegant way of doing things. I clearly need to update my copy of Kakoune and play around with the new features - I haven't updated in almost a year 😅.
I've thought about it, but that would have been an even bigger delta; I figured separation breaks git diff viewability, and can be done whenever we decide. Plus, going back to your concern about being able to figure out what's going on, having to switch between two separate source files probably makes it even harder to follow. I've arrived at placing code in options after a lot of tinkering in the past (as is the case with a lot of "unconventional" approaches). mru-files tries to touch the disk as little as possible (I run live Linux distros from USB sticks; merely switching buffers shouldn't trigger disk activity). Therefore I load code from a .sh file (on disk) into an option (in memory), and use it as a shell prelude. All things said, for a small .kak source like bundle, keeping the prelude in an option seemed like the best choice when tinkering. Yeah, it's unconventional so it causes surprise.
The truth of the matter is, I suppose a lot of the pushback I've given you stems from a fear of the unknown. I'm not exactly the most experienced plugin developer around (this is only my second plugin, actually), so you have a lot more knowledge than I do about what works and what doesn't. I mainly started this as a learning project, along with my WIP kak-dap plugin (that might end up getting done next year at this rate), and this was far more involvement than I was ever expecting anyone else to have in this project.
I am exceedingly grateful for your time and patience contributing to this plugin and dealing with my waffling. Let's get this merged.
Actually, I would have probably over-engineered things, were it not for the design discussions — so please, waffle on, it yields better results :)
I've implemented most of the things discussed in #2
Apologies for going ahead with some things you haven't been able to reply to yet, but I was kind of eager to try it out for my entire setup.
If you prefer to do some things differently, let me know and we can make changes, or remove some of the unwanted features.
I'm going to try one more thing: optionally (
bundle_parallel
) rungit clone
's andgit pull
's in background jobs, then wait for everything to finish, concatenate the logs, and report back to kakoune. As things stand,bundle-install
andbundle-update
can be very slow, block kakoune and scare the user. Plus, yeah, who doesn't like fast response.