search

Shizcow / dmenu-rs

A pixel perfect port of dmenu, rewritten in Rust with extensive plugin support
GNU General Public License v3.0
197 stars 9 forks source link

fix: dmenu_path: create cache if it doesnt exist #56

Closed maheshbansod closed 9 months ago

maheshbansod commented 10 months ago

This PR deviates from the behaviour of the original dmenu_path script so I'm unsure if it would be acceptable in this repo.

Deleting the dmenu_run cache and running dmenu_path is not regenerating the cache in the original dmenu either for me - for both original and dmenu-rs, i get the error that the file doesn't exist.

$ ./stest -flx -n ~/.cache/dmenu_run .
/home/light/.cache/dmenu_run: No such file or directory
dmenu
dmenu_run
dmenu_path
stest

The arch linux wiki says that it should regenerate it when deleted (https://wiki.archlinux.org/title/dmenu#Missing_menu_entries) , but on my system that didn't happen and I was unable to find any code path that regenerates the cache, so here I'm assuming that this problem is not just on my system - please correct me if I'm wrong.

This PR adds a condition that if the cache doesn't exist, then we add the cache before checking for last modified date of the file with stest.

benjaminedwardwebb commented 9 months ago

Thanks for raising this @maheshbansod! It helped me uncover some problems in our stest implementation.

Broadly speaking, I think the changes here are a very reasonable idea. In fact, I think you might consider submitting them upstream to the original dmenu project written in C. However, after reviewing this PR and debugging related issues stest, I don't think we should merge this pull request here for several reasons.

The first is that, based on my read of the project and interactions with @Shizcow, consistency with the original dmenu takes priority (as you already note).

The second is that I believe the behavior you observed was actually due to an inconsistency (i.e., a bug) between the rust implementation of stest here and its original C implementation. I've fixed these inconsistencies in #57. Once that merges, I expect this project's version of dmenu_run will automatically recreate the cache file at ~/.cache/dmenu_run if it's missing, as the original dmenu_run does.

To clarify any confusion, the original dmenu_path script does automatically recreate the cache file at ~/.cache/dmenu_run.

However, it only does so if stest -dqr -n "$cache" $PATH succeeds, which hinges on the behavior of -n. This behavior was inconsistent with the original stest prior to #57. The line also requires some other things in order to succeed, such as having at least a single existing directory in your PATH.

Also note that just because the original stest reports an error message does not mean that its exit code is non-zero.

If you want a lot more detail, I suggest you read the commit messages in #57. There might be an argument that this patch should be submitted upstream as a better way of regenerating the cache file if it's missing. The behavior of the -n and -o flags that the current C implementation relies on to regenerate the cache file is, in my opinion, a little counterintuitive (e.g., reporting an error but not failing; being subtly inconsistent with GNU coreutils' test).

maheshbansod commented 9 months ago

I see. Thank you for the detailed explanation! Closing this one then.