search

beetbox / beets

music library manager and MusicBrainz tagger
http://beets.io/
MIT License
12.57k stars 1.8k forks source link

Update the bash completion script #5301

Closed bal-e closed 2 weeks ago

bal-e commented 2 weeks ago

Some functions used by the bash completion script have become deprecated. This was causing the test suite to break on faster-moving distributions (e.g. Arch Linux and Gentoo). The issue is only noticeable because the tests for the completion function explicitly disallow compatibility implementations from being loaded. Rather than allowing compatibility implementations, I've added some basic logic to the completion script so that older and newer versions of bash-completion can be supported simultaneously.

For some reason, this issue wasn't caught on CI - I think the newer bash-completion hasn't reached the distribution used there. Still, this code is now forward-compatible.

Description

Fixes #5243 (see here).

To Do

I suppose it may be possible to set up tests for this, but that doesn't sound fun or particularly necessary. Still, I'll leave that decision up to the reviewer.

Serene-Arc commented 2 weeks ago

We use ubuntu-latest as the OS for the tests, along with Windows. It does update slower than Arch. But thank you for the PR!

Technically speaking, right now Debian 10 (Buster) is still in LTS, at least until the end of the month (2024-06-30) and it uses bash-completion 2.8, with the next oldest, Bullseye, using v2.11. If there's no way to support older versions other than simply failing and returning with an error code, then I say that this PR should wait until Debian 10 officially passes out of LTS. It's only two weeks and change anyway.

bal-e commented 2 weeks ago

Oh, I had no idea 2.8 was still in use! I set the minimum to 2.10 somewhat arbitrarily (I had no idea how long this script has been working for successfully). I'm not planning to make use of any 2.10-plus features for any more changes to the script, so I'll just downgrade the version requirement to 2.8, with a note for the next person interested.

Until now, I hadn't been able to pass the test suite locally, which made development much more irritating. I want to get this merged soon so that others don't have to go down the same rabbit hole.

bal-e commented 2 weeks ago

Thanks @Serene-Arc!

wisp3rwind commented 2 weeks ago

Wow, thanks for tackling this! I've been seeing these test failures locally for quite some time, but never bothered to investigate.