Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically rebuild custom assets when necessary #2085

Open
Enselic opened this issue Feb 23, 2022 · 7 comments
Open

Automatically rebuild custom assets when necessary #2085

Enselic opened this issue Feb 23, 2022 · 7 comments
Labels
feature-request New feature or request

Comments

@Enselic
Copy link
Collaborator

Enselic commented Feb 23, 2022

Currently, if a user makes use of custom assets and then installs a new 0.x.0 version of bat, bat will stop working. That is not an ideal user experience. It would be good if we could make this use case smoother, e.g. by automatically doing the equivalent of running bat cache --build for the user.

Step-by-step

Here is a step by step against the git repo, but regular users will get this problem too via regular releases from us and e.g. package manager updates of the bat app.

  1. cargo run -- cache --build --blank --source assets
  2. Bump 0.x.0 version in Cargo.toml, e.g. from 0.19.0 to 0.20.0
  3. cargo run -- examples/simple.rs

Expected result

bat prints examples/simple.rs

Actual result

[bat error]: The binary caches for the user-customized syntaxes and themes in
'/Users/martin/.cache/bat' are not compatible with this version of bat (0.20.0). To solve this,
either rebuild the cache (bat cache --build) or remove the custom syntaxes/themes (bat
cache --clear).
For more information, see:

  https://github.com/sharkdp/bat#adding-new-syntaxes--language-definitions
@sharkdp
Copy link
Owner

sharkdp commented Mar 6, 2022

I did consider this when first implementing the above semver-incompatibility detection and error message. My only concern is that we can not know if the binary user assets and the source user assets are still "compatible". This might be a contrived case, but what I was thinking about is the following scenario:

  • user puts custom assets in $(bat --config-dir)/syntax
  • user runs bat cache --build
  • user somehow modifies files in $(bat --config-dir)/syntax (e.g.: removes source files, or adds some source files but then decides to not run bat cache --build)
  • new version of bat comes along that simply rebuilds the binary assets from source and changes the user behavior.

I therefore opted for the explicit version rather than the implicit/automatic one.

But I'm happy to reconsider this. I might have been a bit over-cautious.

@awerlang
Copy link

* user _somehow_ modifies files in `$(bat --config-dir)/syntax` (e.g.: removes source files, or adds some source files but then decides to not run `bat cache --build`)

* new version of `bat` comes along that simply rebuilds the binary assets from source and changes the user behavior.

What if bat manages its caches transparently. Users should only have control over the plain configuration files. User behavior is governed by its config files.

@Enselic
Copy link
Collaborator Author

Enselic commented Mar 27, 2022

You mean bat should check the timestamp of each custom theme and custom syntax, and then compare that with the timestamp of the built custom assets (a.k.a. "the cache"), and if any asset is newer than the cache, then the cache is automatically rebuilt?

I think that is a good solution. My only concern is what impact if would have on startup time. But we already today read the metadata file on each startup, so I suspect doing something akin to above would not have that big of an impact.

If we can get that to work, we potentially could get rid of the cache subcommand entirely, which would be nice. It would fix #1726 for example.

I think it would also take care of the scenario you describe David, because as soon as the user "breaks" their custom assets, they would learn about it right away. (Or to be precise, the next time they invoke bat.)

@awerlang
Copy link

You mean bat should check the timestamp of each custom theme and custom syntax, and then compare that with the timestamp of the built custom assets (a.k.a. "the cache"), and if any asset is newer than the cache, then the cache is automatically rebuilt?

I'm not advocating for any solution in particular here. An actual solution should find a cached asset that semver matches the running bat, and source checksum also matches. If matching fails then rebuild and add or replace cache.

I also linked dandavison/delta#1018 since other programs might depend on bat caches but support a different library version. IMO programs should not be sharing caches this way without a proper solution as outlined above. This is important as Rust doesn't support dynamic linking, in contrast to how C libraries would be packaged. I also appreciate ~/.cache/bat can be empty and it just works.

@cyqsimon
Copy link
Contributor

cyqsimon commented Oct 7, 2022

I would like to suggest updating the completion scripts to prevent autocompleting the cache subcommand alongside file names. This will eventually be done when this issue is closed, but I believe there is good reason to do it immediately:

Imagine this situation (which gets me almost every day):

  1. I make a new folder and cd into it
  2. I use cp or mv to copy/move in a file, let's call it foo.bar
  3. I want to view foo.bar with bat
  4. Knowing there is only one file in this directory, I type bat , hit tab and enter, expecting the foo.bar to get completed
  5. Nothing got completed because Bash doesn't know whether to complete cache or foo.bar
  6. bat got run, so bat hangs while waiting for input on stdin

You can argue that this is user error on my part, or that my fingers are too fast for my own good (which could be true), but I would insist that the completion script is at fault here. The reason is simply that this behaviour is dissimilar to most other commonly-used tools (e.g. cat, diff, your favourite editor, etc.), and is certainly not what the user expects.

And in return for the constant frustration, the benefit is clearly minimal. The bat-cache subcommand is very rarely used if ever (at least in my experience), so its existence in the completion script is more of a stumbling block than anything else.

Therefore I think it's a good idea to remove it right now from all completion scripts. Should be only a few lines of deletion. If people agree with this opinion then I'm happy to go ahead and create a PR.

P.s. I'm only suggesting the removal of the autocompletion of the word cache, not the removal of the autocompletion of bat-cache's flags. These don't really cause trouble and can stay until the subcommand gets removed entirely.

@keith-hall
Copy link
Collaborator

Therefore I think it's a good idea to remove it right now from all completion scripts. Should be only a few lines of deletion. If people agree with this opinion then I'm happy to go ahead and create a PR.

I personally have no objections.

@acrewdson
Copy link

I would like to suggest updating the completion scripts to prevent autocompleting the cache subcommand alongside file names.

big +1, this would be a huge improvement in usability – thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants