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

Use Taglib1 package on Linux if found #12760

Merged
merged 5 commits into from
Feb 12, 2024
Merged

Use Taglib1 package on Linux if found #12760

merged 5 commits into from
Feb 12, 2024

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Feb 8, 2024

This allows to use the https://aur.archlinux.org/packages/taglib1 package on Linux Arch.
This is a workaround for #12790

@mxmilkiib
Copy link
Contributor

That builds 2.4 for me!

(Though I would prefer to be using main.)

@daschuer
Copy link
Member Author

daschuer commented Feb 8, 2024

Cleaned the commit history. I think it is reasonable to make this a 2.4.0 PR.

@daschuer daschuer added this to the 2.4.0 milestone Feb 8, 2024
@vlada-dudr
Copy link
Contributor

Wouldn't be better to parametric? Like -DTAGLIB_PKGCONFIG_PATH=/wherever/it/is or we don't care because it is temporary solution?

@daschuer
Copy link
Member Author

daschuer commented Feb 8, 2024

This can already be done by just setting the environment variable PKG_CONFIG_PATH

@toszlanyi
Copy link
Contributor

That builds 2.4 for me!

also confirming this change builds nicely now on my EndeavourOS (Arch) with taglib2 and taglib1 installed side by side. Thank you @daschuer

@Holzhaus
Copy link
Member

I think should be dealt with by the package/AUR maintainer, not in our code. taglib1 is not even an official Arch package, it's a user-maintained package from the AUR. I'd rather not add workaround for that.

Comment on lines +51 to +52
# priorize the taglib1 package introduced in https://aur.archlinux.org/packages/taglib1
set(ENV{PKG_CONFIG_PATH} "/usr/lib/taglib1/lib/pkgconfig/:$ENV{PKG_CONFIG_PATH}")
Copy link
Member Author

Choose a reason for hiding this comment

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

@Holzhaus Only these two lines are ARCH specific. The rest is required to have two version of taglib installed. We may remove it but on the other hand it is convenient for our contributors and does not really hurt.

@daschuer
Copy link
Member Author

Which distro do you use? has Taglib2 landed there?

@daschuer
Copy link
Member Author

This is the situation: https://repology.org/project/taglib/versions

@daschuer
Copy link
Member Author

It looks like Debian will allow using a taglib1 and taglib2 package but the development files are conflicting right now.

icedream added a commit to icedream/customizepkg-config that referenced this pull request Feb 11, 2024
Force mixxx-git to use the taglib1 AUR package files by incorporating patches from mixxxdj/mixxx#12760.

The patches had to be slightly adapted to account for some changes on the main branch.
@ywwg
Copy link
Member

ywwg commented Feb 11, 2024

I am ok with this kind of hack in order to get a release out -- we can do a "proper" fix later if possible,

@ywwg
Copy link
Member

ywwg commented Feb 11, 2024

I do not know anything about this issue, @Holzhaus are you ok for us to merge this in or is your objection blocking?

@Holzhaus
Copy link
Member

No, I'm not blocking this but I still think this is unnecessary and the taglib/ prefix removal from the includes makes the includes harder to understand what library it belongs to.

A patch like that can easily be added to the Mixxx AUR package (which I maintain).

For the mixxx package in the Arch package repository, the package maintainer could apply it as well if deemed necessary. However, the mixxx package for 2.4 on Arch Linux will definitely not depend on the taglib1 AUR package. Either the package maintainer creates a new distro package for legacy TagLib and adds that to the distro repositories (and we don't know if these patches will even be necessary for that package), or mixxx is removed from the Arch repos because they don't want to add such a package.

That's why I don't think it makes sense to hack workarounds for specific distros here. That's something package maintainers are responsible for.

@daschuer
Copy link
Member Author

Thank you @Holzhaus

The removal of the "taglib/" include prefix is required because of this upstream PR :taglib/taglib@bd4c9cb
We don't know the include folder package maintainers will choose. Now, we just follow where pkgconf points us. Before we ended up in a mixture of headers of both trees.

@ywwg
Copy link
Member

ywwg commented Feb 12, 2024

merging mostly just to get the release moving. We can come back to the issue later and maybe move per-distro patches to their respective packaging repos

@ywwg ywwg merged commit b61811c into mixxxdj:2.4 Feb 12, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants