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.

Sign up for GitHub

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

Jump to bottom

Feature request: drop dependency for Perl in key-bindings.bash #3077

Closed
5 of 10 tasks
jlebon opened this issue Dec 7, 2022 · 10 comments
Closed
5 of 10 tasks

Feature request: drop dependency for Perl in key-bindings.bash #3077

jlebon opened this issue Dec 7, 2022 · 10 comments

Comments

@jlebon
Copy link

jlebon commented Dec 7, 2022 •
edited
Loading

  • I have read through the manual page (man fzf)
  • I have the latest version of fzf
  • I have searched through the existing issues

Info

  • OS
    • Linux
    • Mac OS X
    • Windows
    • Etc.
  • Shell
    • bash
    • zsh
    • fish

Problem / Steps to reproduce

__fzf_history__() in key-bindings.bash requires Perl. In more minimal environments, Perl may not be installed. It would be nice to be able to drop that dependency. Looking at the exact code there, it seems like it's just doing some frequency counting of the bash command history. Could we have that be built into fzf perhaps and gated behind a flag?
step- added a commit to step-/fzf that referenced this issue May 14, 2023
@step-
replace perl with awk in the bash history widget
31b3a21 
While awk is POSIX, perl isn't pre-installed on all *nix flavors. This
commit replaces perl with awk thus it eliminates the perl dependency.

This commit passed the current test suite with zero errors :
* `make error` => all test sections 'ok'
* `make docker-test` =>
  Finished in 50.929904s, 4.1822 runs/s, 35.3427 assertions/s.
  213 runs, 1800 assertions, 0 failures, 0 errors, 0 skips

See also:  junegunn#2777 junegunn#1934 # junegunn#3077 junegunn#2260

Note: unlike the original perl script, which gets `$HISTCOUNT` from
the environment, the awk script reads the initial history count from
line 1 of stdin. Then follow the history list, and finally a `\t`.
The reason for `\t` is to fix a small, pre-existing issue I noticed
while developing this patch (see the PR associated with this commit
for additional information).
@step- step- mentioned this issue May 14, 2023
Closed
@glensc
Copy link

glensc commented Sep 8, 2023 •
edited
Loading

this doesn't seem like big deal to rewrite in awk or so.

EDIT: the PR is already there: #3295

@jlebon
Copy link
Author

jlebon commented Sep 8, 2023

Looks like the latest effort is stuck on performance issues: #3313 (comment). I added a suggestion there.

@step-
Copy link
Contributor

step- commented Sep 13, 2023

Author of #3395 (closed), #3310 (bash) and #3313 (awk) here. I read your suggestion in #3310. You're suggesting to implement an ad-hoc filter that would do what the perl filter (or my awk filter) does now. Am I right?

@step- step- mentioned this issue Sep 13, 2023
Merged
@jlebon
Copy link
Author

jlebon commented Sep 13, 2023

Author of #3395 (closed), #3310 (bash) and #3313 (awk) here. I read your suggestion in #3310. You're suggesting to implement an ad-hoc filter that would do what the perl filter (or my awk filter) does now. Am I right?

Right, I'm suggesting implementing the filter in golang behind some hidden switch. But I'm not a maintainer :) I would try to get @junegunn's opinion on it before doing this.

@step-
Copy link
Contributor

step- commented Sep 22, 2023

can be closed due to #3313 merge

@kaddkaka
Copy link

It seems like perl is still the default option and awk is only a fallback.

  1. Is there a reason to keep the perl dependency?
  2. Are there any pros to using perl over awk?

(I have perl but my perl environment is a little bit broken (complains about locale settings), I will modify my local key-bindings.bash to always select awk for now.)

@step-
Copy link
Contributor

step- commented Sep 27, 2024

TLDR; #3313 (comment)

@glensc
Copy link

glensc commented Sep 28, 2024

@kaddkaka broken about locale, add LC_ALL=C before perl command to mute them. probably worth sending PR to this repo too.

@kaddkaka
Copy link

@kaddkaka broken about locale, add LC_ALL=C before perl command to mute them. probably worth sending PR to this repo too.

Thanks.I'm not confident to do a PR because I don't understand why that would work.

@glensc
Copy link

glensc commented Sep 29, 2024

you can test that with your environment it no longer prints locale init errors:

perl -e 1
LC_ALL=C perl -e 1

perl would need locales as some code depends on locale data. regexes? but the current script doesn't seem to contain something that would need locales, so it seems safe :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@glensc @junegunn @step- @kaddkaka @jlebon