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

[spec] Add Recommends %{_bindir}/sqlite3 for bash-completion for Fedora #1985

Merged

Conversation

grumpey
Copy link
Contributor

@grumpey grumpey commented Sep 4, 2023

#1817 added the ability to utilize sqlite cache when available.
https://discussion.fedoraproject.org/t/why-bash-auto-completion-is-so-slow-with-dnf/88944 and
https://discussion.fedoraproject.org/t/dnf-autocompletion-is-too-slow/64038 Are users noting that bash completion is slow without sqlite installed.

= changelog =
msg: [spec] Add Recommends %{_bindir}/sqlite3 for bash-completion for Fedora
type: enhancement
related: #1817
related: https://discussion.fedoraproject.org/t/why-bash-auto-completion-is-so-slow-with-dnf/88944
related: https://discussion.fedoraproject.org/t/dnf-autocompletion-is-too-slow/64038

@j-mracek j-mracek self-assigned this Sep 14, 2023
@j-mracek
Copy link
Member

I checked the internal implementation and I think we don't need the recommend:

See:

    if ! _dnf_is_path "$cur"; then
        if [ -r "$__dnf_cache_file" ] && [ -x /usr/bin/sqlite3 ]; then
            case "$cmd$*" in
            listinstalled|listupdates|\
            dg|downgrade|\
            ri|rei|reinstall|\
            rm|remove*|erase*|\
            um|u-m|upgrade|upgrade-n*|\
            up|up-min|update|update-n*)
                COMPREPLY+=( $(_dnf_query_db installed "$cur") )
                ;;
            *)
                COMPREPLY+=( $(_dnf_query_db available "$cur") )
                ;;
            esac
        else
            COMPREPLY+=( $(compgen -W "$( _dnf_commands_helper $cmd "$@" "$cur" )") )
        fi
    fi

If I am not mistaken if sqlite3 is not present it uses an alternative approach. May I ask you for additional reasons why we should add the recommend?

@grumpey
Copy link
Contributor Author

grumpey commented Sep 17, 2023

If I am not mistaken if sqlite3 is not present it uses an alternative approach. May I ask you for additional reasons why we should add the recommend?

The complaint here centers around the speed of the completion.
In some instances folks have reported a noticeable delay between 1-4 seconds when waiting for the tab completion or failures to complete.

I suspect some of this has to do with the cache being expired, but this could be mitigated somewhat by having a better internet connection. I do think this was worse back when I was utilizing cellular for internet.

When the cache is up to date, completion still appears to be a slower (.5 - 1 second) without sqlite on my machine. (AMD Ryzen 5 5600G )

Thanks

@j-mracek
Copy link
Member

Ok, good point.

dnf.spec Outdated
@@ -85,6 +85,8 @@ Requires: python3-%{name} = %{version}-%{release}
%if 0%{?rhel} && 0%{?rhel} <= 7
Requires: python-dbus
Requires: %{_bindir}/sqlite3
%elif 0%{?fedora}
Recommends: (%{_bindir}/sqlite3 if bash-completion)
Copy link
Member

Choose a reason for hiding this comment

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

I am proposing to add additional condition (%{_bindir}/sqlite3 if (bash-completion and python3-dnf-plugins-core)).
If python3-dnf-plugins-core is not installed then the database cannot be updated and bash completion provides outdated results.

Be honest this approach is not perfect, because it uses resources during RPM transaction. DNF5 uses an alternative approach without external database.

dnf.spec Outdated
@@ -85,6 +85,8 @@ Requires: python3-%{name} = %{version}-%{release}
%if 0%{?rhel} && 0%{?rhel} <= 7
Requires: python-dbus
Requires: %{_bindir}/sqlite3
%elif 0%{?fedora}
Recommends: (%{_bindir}/sqlite3 if (bash-completion && python3-dnf-plugins-core))
Copy link
Member

Choose a reason for hiding this comment

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

I am really sorry but operator && is not supported. See https://rpm-software-management.github.io/rpm/manual/boolean_dependencies.html. May I ask you to replace && by and?

@grumpey
Copy link
Contributor Author

grumpey commented Sep 18, 2023 via email

@grumpey grumpey force-pushed the Recommends_For_SQLITE3 branch 3 times, most recently from 05fbdf7 to ab3ba85 Compare September 18, 2023 23:16
@j-mracek j-mracek merged commit 2d2047e into rpm-software-management:master Sep 20, 2023
3 checks passed
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.

None yet

2 participants