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

remove perl dependency for bash #1934

Closed
wants to merge 12 commits into from
Closed

remove perl dependency for bash #1934

wants to merge 12 commits into from

Conversation

rickywu
Copy link

@rickywu rickywu commented Mar 21, 2020

refer to #1931

juse use builtin histtory and sort by reverse order, and use sed and tr to replace perl

@junegunn
Copy link
Owner

Please make sure that the existing tests all pass. (make docker-test)
Hmm, I don't think there's a good way to handle multi-line commands with sed as we do now.

@rickywu
Copy link
Author

rickywu commented Mar 25, 2020

OK, both sed and awk can do this

fc -lr -2147483648 | sed -r -e 's/^([0-9]+\t) +/\1/g;' | sed -z -r -e 's/\n([0-9]+\t)/\x00/g;'

or

fc -lr -2147483648 | awk 'BEGIN{RS="\n[0-9]+\t ";ORS="\x00";OFS=""}NR==1{$0=gensub(/^([0-9]+\t) +/,"\\1","g",$0)}{print num,$0}{num=gensub(/\n([0-9]+\t) +/,"\\1","g",RT)}'
Here are the two options test result:

docker run -it fzf-arch
[exited]
Run options: --seed 37568

Running:

............................................................................................................................................

Finished in 68.682246s, 2.0384 runs/s, 1.7181 assertions/s.

140 runs, 118 assertions, 0 failures, 0 errors, 0 skips

One problem is sed -z option is only available version 4.2.2+ which released at Sat, 22 Dec 2012

New option -z (--null-data) to separate lines by ASCII NUL characters.

@junegunn
Copy link
Owner

One problem is sed -z option is only available version 4.2.2+ which released at Sat, 22 Dec 2012

Then we can't use it, macOS Catalina ships with old BSD sed (2005).

$ sed -z
sed: illegal option -- z

@@ -290,7 +290,7 @@ d_cmds="${FZF_COMPLETION_DIR_COMMANDS:-cd pushd rmdir}"
a_cmds="
awk cat diff diff3
emacs emacsclient ex file ftp g++ gcc gvim head hg java
javac ld less more mvim nvim patch perl python ruby
javac ld less more mvim nvim patch python ruby
Copy link
Owner

Choose a reason for hiding this comment

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

This is unrelated.

@rickywu
Copy link
Author

rickywu commented Mar 26, 2020

gensub is not available for BSD awk on mac os, just hold on, I'll find a better solution

@junegunn
Copy link
Owner

Yeah, thanks. Is there a reason you want to remove perl dependency? Perl is available on virtually every Unixy system nowadays.

@rickywu
Copy link
Author

rickywu commented Mar 26, 2020

I‘ve tested awk with sub function works well on mac OS and passed docker-test, you can review now.

The reason is I want to use fzf in docker container and
most of bash distribution include sed and awk but except perl, such as alpine Linux distribution

@junegunn
Copy link
Owner

junegunn commented Mar 27, 2020

works well on mac OS

While it works with GNU awk, which can be installed using Homebrew (brew install gawk -> /usr/local/bin/awk), it doesn't work with the default awk from macOS (/usr/bin/awk). Can you check again? Thanks.

@rickywu
Copy link
Author

rickywu commented Mar 28, 2020

Yes, you are right.
I've checked, for compatible BSD bash, the only option is use perl.
So I changed check if gnu awk available, otherwise use perl, is this solution acceptable?

@junegunn
Copy link
Owner

Thanks, but I'm not a fan of this code branch. (FYI, we rely on Perl even more since 18261fe)

Minimizing dependency is nice, but I'd just use Perl in this case as I don't want to increase code complexity and I don't think it's too much to require Perl on interactive command-line environments.

The reason is I want to use fzf in docker container and
most of bash distribution include sed and awk but except perl, such as alpine Linux distribution

That's a valid point but such Docker images are mainly for building lightweight containers rather than interactive command-line environments. So I think it's forgivable that interactive fzf scripts don't work in those images out of the box without additional installation steps. Perl on Alpine is quite small, so I suggest that you just install Perl when you build your Docker containers.

@junegunn junegunn closed this Mar 30, 2020
step- added a commit to step-/fzf that referenced this pull request May 14, 2023
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants