Skip to content

Unbundle file engine binaries from plugin - #282

Open
jyscao wants to merge 1 commit into
vim-ctrlspace:masterfrom
jyscao:unbundle_bins
Open

Unbundle file engine binaries from plugin#282
jyscao wants to merge 1 commit into
vim-ctrlspace:masterfrom
jyscao:unbundle_bins

Conversation

@jyscao

@jyscao jyscao commented Nov 21, 2020

Copy link
Copy Markdown
Collaborator

Instead making users download 4 file engine binaries (3 of which they will never use or even all 4 if their OS+arch doesn't match any of them), it makes more sense to use a post install script to download only the binary applicable to their system.

@jyscao
jyscao requested a review from Konfekt November 21, 2020 03:49
@jyscao

jyscao commented Nov 21, 2020

Copy link
Copy Markdown
Collaborator Author

Additional minor improvements:

  • currently, the script gets triggered on all plugin updates even if the binaries themselves are unchanged. Proposed solution: add checksums for all fine engine binaries, then only re-download if checksums differ from user's existing binary in their vim-ctrlspace/bin

  • add powershell/batch script to provide auto-download of file_engine_windows_amd64.exe for Windows users

@Konfekt Konfekt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very sensible improvement; thanks for the good work!

Comment thread README.md Outdated
Comment thread autoload/ctrlspace/context.vim Outdated

if s:conf.FileEngine ==# "auto"
let s:conf.FileEngine = s:detectEngine()
else

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above proviso.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I been meaning to submit an issue outlining a couple of my main medium to long-term plans/ideas for the project going forward, you'll know it when you see it.

Just as a preview though, one major point I want to raise is that, for an eventual major release we should remove some unnecessary options (which ones are deemed so is ofc up for discussion and should be agreed upon before the changes are committed).

Comment thread bin/fetch-engine.sh Outdated
Comment thread bin/fetch-engine.sh Outdated
@jyscao

jyscao commented Nov 22, 2020

Copy link
Copy Markdown
Collaborator Author

@Konfekt I added a MD5 checksum check, and added some more details to the README. Lmk what you think; I think it's probably more or less good to be merged.

Although I'm not too sure if we should just remove those 4 binaries from tracking immediately. Since if that's done, next time people pull the update, it'll be deleted from their plugin, but if the script didn't work correctly, then they might get confused. But without removing the binaries from git tracking, then it defeats the whole point of this PR. What you think?

@Konfekt

Konfekt commented Nov 22, 2020

Copy link
Copy Markdown
Collaborator

Well, this change is abrupt, but for the better. The spell download vimscript file

https://github.com/vim/vim/blob/master/runtime/autoload/spellfile.vim

serves as a template to implement it without any need for vim-plug on every OS.

* add post-install shell script to auto fetch file engine bin
* use MD5 to prevent unnecessary re-downloading
* update 'Installation' section of README
* remove existing bins from git tracking (don't delete bins for now)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants