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

Fail early in case Taglib 2.0 is found #12709

Merged
merged 2 commits into from
Feb 4, 2024
Merged

Fail early in case Taglib 2.0 is found #12709

merged 2 commits into from
Feb 4, 2024

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Feb 1, 2024

This makes the build error because of Taglib 2.0 explicit.

#12708

I am strictly against hacking a support in to 2.4-beta, because it requires a good amount of testing.
Any issue will put user data on a risk

See also the discussion about the new " / " separator in multi-valued fields
#12613

@github-actions github-actions bot added the build label Feb 1, 2024
@daschuer daschuer added this to the 2.4.0 milestone Feb 3, 2024
@daschuer
Copy link
Member Author

daschuer commented Feb 4, 2024

Can we merge this soon? We really have to avoid brute force patches that make Mixxx build with Taglib 2.0 without proper testing-

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

how likely is it that this will cause issues for package maintainers? Is is reasonable to expect taglib>=1.11 to be available everywhere?

CMakeLists.txt Outdated Show resolved Hide resolved
@daschuer
Copy link
Member Author

daschuer commented Feb 4, 2024

It is very likely that everyone has Taglib 1.11 above since even Ubuntu Focal 20.04 LTS has it.

@daschuer
Copy link
Member Author

daschuer commented Feb 4, 2024

This will likely cause trouble for the Linux Arch and OpenBSD package maintainers, because they have decided to not put Taglib 2.0 in a separate namespace. This is not an issue for Tagging software out there, but it is an issue for all Media players that sync their internal database to the file tags.
I am not sure how big the issue is, but we should not rush into it for our 2.4.0 release that is overdue anyway.

Fixing this seems to be the most pressing issue for 2.4.1

@Swiftb0y
Copy link
Member

Swiftb0y commented Feb 4, 2024

Great. so Mixxx 2.4.1 shipping taglib 2 is reasonable milestone?

@daschuer
Copy link
Member Author

daschuer commented Feb 4, 2024

The issue is that all users of these bleeding edge systems can't even not use Mixxx 2.3.6. because of the binary incompatibility of Taglib 2.0.

@daschuer
Copy link
Member Author

daschuer commented Feb 4, 2024

Done.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM.

@Swiftb0y Swiftb0y merged commit 4cfb8c4 into 2.4 Feb 4, 2024
19 of 20 checks passed
@Swiftb0y Swiftb0y deleted the taglib_20_fail branch February 4, 2024 13:35
@mxmilkiib
Copy link
Contributor

2.5 build fails on Arch currently.

I installed taglib1 from the AUR, but it still fails..

@daschuer
Copy link
Member Author

daschuer commented Feb 5, 2024

Can you post the terminal output of the failure?

@mxmilkiib
Copy link
Contributor

[ 21%] Building CXX object CMakeFiles/mixxx-lib.dir/src/widget/weffectchain.cpp.o
/home/milk/pkgs/mixxx-git-debug/src/mixxx/src/track/taglib/trackmetadata_ape.cpp: In function ‘bool mixxx::taglib::ape::importCoverImageFromTag(QImage*, const TagLib::APE::Tag&)’:
/home/milk/pkgs/mixxx-git-debug/src/mixxx/src/track/taglib/trackmetadata_ape.cpp:62:56: error: ‘const class TagLib::APE::Item’ has no member named ‘value’; did you mean ‘values’?
   62 |                 tag.itemListMap()["COVER ART (FRONT)"].value();
      |                                                        ^~~~~
      |                                                        values
make[3]: *** [CMakeFiles/mixxx-lib.dir/build.make:5718: CMakeFiles/mixxx-lib.dir/src/track/taglib/trackmetadata_ape.cpp.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [CMakeFiles/Makefile2:243: CMakeFiles/mixxx-lib.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:302: CMakeFiles/mixxx.dir/rule] Error 2
make: *** [Makefile:212: mixxx] Error 2
==> ERROR: A failure occurred in build().
    Aborting...
makepkg -si --noextract  113.20s user 16.50s system 441% cpu 29.384 total

@vlada-dudr
Copy link
Contributor

diff --git a/src/track/taglib/trackmetadata_ape.cpp b/src/track/taglib/trackmetadata_ape.cpp
index c370c6a7f9..b4af3b5afb 100644
--- a/src/track/taglib/trackmetadata_ape.cpp
+++ b/src/track/taglib/trackmetadata_ape.cpp
@@ -59,7 +59,7 @@ bool importCoverImageFromTag(QImage* pCoverArt, const TagLib::APE::Tag& tag) {
     if (tag.itemListMap().contains("COVER ART (FRONT)")) {
         const TagLib::ByteVector nullStringTerminator(1, 0);
         TagLib::ByteVector item =
-                tag.itemListMap()["COVER ART (FRONT)"].value();
+                tag.itemListMap()["COVER ART (FRONT)"].binaryData();
         int pos = item.find(nullStringTerminator); // skip the filename
         if (++pos > 0) {
             const TagLib::ByteVector data(item.mid(pos));

Works for me. But if you crashed on this likely means that mixxx builds agains taglib 2.0

@mxmilkiib
Copy link
Contributor

mxmilkiib commented Feb 8, 2024

Works for me.

What works for you? Building 2.5-alpha on an up-to-date Arch machine with taglib1 from the AUR?

likely means that mixxx builds agains taglib 2.0

2.0 is installed currently, hmm.

FWIW/clarity, here is the file listing of both taglib and taglib1;

taglib /usr/
taglib /usr/bin/
taglib /usr/bin/taglib-config
taglib /usr/include/
taglib /usr/include/taglib/
taglib /usr/include/taglib/aifffile.h
taglib /usr/include/taglib/aiffproperties.h
...
taglib /usr/include/taglib/xmfile.h
taglib /usr/include/taglib/xmproperties.h
taglib /usr/lib/
taglib /usr/lib/cmake/
taglib /usr/lib/cmake/taglib/
taglib /usr/lib/cmake/taglib/taglib-config-version.cmake
taglib /usr/lib/cmake/taglib/taglib-config.cmake
taglib /usr/lib/cmake/taglib/taglib-targets-noconfig.cmake
taglib /usr/lib/cmake/taglib/taglib-targets.cmake
taglib /usr/lib/libtag.so
taglib /usr/lib/libtag.so.2
taglib /usr/lib/libtag.so.2.0.0
taglib /usr/lib/libtag_c.so
taglib /usr/lib/libtag_c.so.2
taglib /usr/lib/libtag_c.so.2.0.0
taglib /usr/lib/pkgconfig/
taglib /usr/lib/pkgconfig/taglib.pc
taglib /usr/lib/pkgconfig/taglib_c.pc
taglib1 /usr/
taglib1 /usr/lib/
taglib1 /usr/lib/libtag.so.1
taglib1 /usr/lib/libtag.so.1.19.1
taglib1 /usr/lib/libtag_c.so.0
taglib1 /usr/lib/libtag_c.so.0.0.0
taglib1 /usr/lib/taglib1/
taglib1 /usr/lib/taglib1/bin/
taglib1 /usr/lib/taglib1/bin/taglib-config
taglib1 /usr/lib/taglib1/include/
taglib1 /usr/lib/taglib1/include/taglib/
taglib1 /usr/lib/taglib1/include/taglib/aifffile.h
taglib1 /usr/lib/taglib1/include/taglib/aiffproperties.h
...
taglib1 /usr/lib/taglib1/include/taglib/xmfile.h
taglib1 /usr/lib/taglib1/include/taglib/xmproperties.h
taglib1 /usr/lib/taglib1/lib/
taglib1 /usr/lib/taglib1/lib/libtag.so
taglib1 /usr/lib/taglib1/lib/libtag_c.so
taglib1 /usr/lib/taglib1/lib/pkgconfig/
taglib1 /usr/lib/taglib1/lib/pkgconfig/taglib.pc
taglib1 /usr/lib/taglib1/lib/pkgconfig/taglib_c.pc

Edit: https://aur.archlinux.org/packages/taglib1#comment-955407

@daschuer
Copy link
Member Author

daschuer commented Feb 8, 2024

This should allow to use the new taglib1 package: #12760

Holzhaus added a commit to Holzhaus/mixxx that referenced this pull request Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants