CI: Bump taglib to 2.0

[robinst]

Bump TAGLIB_VERSION in CI build to 2.0. Let's see what happens.

[robinst]

ERROR:  Error installing bundler:
	The last version of bundler (>= 0) to support your Ruby & RubyGems was 2.4.22. Try installing it with `gem install bundler -v 2.4.22`
	bundler requires Ruby version >= 3.0.0. The current ruby version is 2.7.8.225.
Error: Process completed with exit code 1.

Looks like Ruby 2.7 is too old for current bundler versions (newer version is used because I had to bump the Ubuntu version). Should we drop support for it with taglib-ruby 2.0? What would be a good minimum Ruby version to support, 3.0.0?

[jacobvosmaer]

Looking at https://www.ruby-lang.org/en/downloads/

It seems reasonable to me to make Ruby 3.0 the minimum we support in taglib-ruby 2. If people are on a distro that nudges them towards taglib 2, then wouldn't that distro also nudge away from an obsolete Ruby like 2.7?

[robinst]

Thanks! Agreed. Based on that, maybe we could even go with 3.1 as minimum.

[robinst]

Ok, installation works now, now swig generation fails with this:

/home/runner/work/taglib-ruby/taglib-ruby/tmp/x86_64-linux/taglib-2.0/include/taglib/tag.h:248: Error: Syntax error - possibly a missing semicolon (';').

[ufleisch]

I had a look at the build with TagLib 2.0. Unfortunately, I do not know a lot about Ruby and SWIG, but I made some fixes so that nearly all tests pass. A few questions remain:

• I patched TagLib 2.0 like this. Adding virtual to the StreamTypeResolver could be added in TagLib, although it is redundant, as long as it makes SWIG work. However, adding a real operator= to MP4::Item is not possible. Can this be fixed in the SWIG/Ruby/mkmf build system?

sed -i 's/^\(\s*\)~StreamTypeResolver() override = 0;/\1virtual ~StreamTypeResolver() override = 0;/' \
  $PWD/tmp/$PLATFORM/taglib-$TAGLIB_VERSION/include/taglib/fileref.h
if ! grep -q 'operator==' $PWD/tmp/$PLATFORM/taglib-$TAGLIB_VERSION/include/taglib/mp4item.h; then
  sed -i 's/^\(\s*\)\(bool isValid() const;\)$/\1\2\n\1bool operator==(const Item \&other) const { return false; }/' \
    $PWD/tmp/$PLATFORM/taglib-$TAGLIB_VERSION/include/taglib/mp4item.h
fi

• In the way how taglib-ruby is built in the Github workflow, it is linked to the system TagLib, and not to the TagLib 2.0 in the TAGLIB_DIR. Can this be fixed? I had to use a hack to temporarily remove the system TagLib.

sudo mv /lib/x86_64-linux-gnu/libtag.so.1 /lib/x86_64-linux-gnu/libtag.so.1.disabled
TAGLIB_DIR=$PWD/tmp/$PLATFORM/taglib-$TAGLIB_VERSION bundle exec rake compile
sudo mv /lib/x86_64-linux-gnu/libtag.so.1.disabled /lib/x86_64-linux-gnu/libtag.so.1

• Some tests still fail, somehow I did not manage to pass TagLib::ID3v2::V3 to save(). The other errors are about MP4::ItemMap. How can this be fixed?

Here you find my changes as a diff, I did not make a PR because it does not fully solve the problem, but these changes should be one step forward.

From 71084015a166eaae388b7216fc48774036f9d2b2 Mon Sep 17 00:00:00 2001
From: Urs Fleisch <[email protected]>
Date: Wed, 20 Mar 2024 19:09:21 +0100
Subject: Fix build with TagLib 2.0


diff --git a/ext/taglib_aiff/taglib_aiff.i b/ext/taglib_aiff/taglib_aiff.i
index 410fb75..276406a 100644
--- a/ext/taglib_aiff/taglib_aiff.i
+++ b/ext/taglib_aiff/taglib_aiff.i
@@ -4,6 +4,8 @@
 #include <taglib/aifffile.h>
 #include <taglib/aiffproperties.h>
 #include <taglib/id3v2tag.h>
+#include <taglib/tpicturetype.h>
+using namespace TagLib;
 %}
 
 %include "../taglib_base/includes.i"
diff --git a/ext/taglib_base/includes.i b/ext/taglib_base/includes.i
index 097545a..3743974 100644
--- a/ext/taglib_base/includes.i
+++ b/ext/taglib_base/includes.i
@@ -5,6 +5,7 @@
 #define TAGLIB_IGNORE_MISSING_DESTRUCTOR
 #define TAGLIB_DEPRECATED
 #define TAGLIB_MSVC_SUPPRESS_WARNING_NEEDS_TO_HAVE_DLL_INTERFACE
+%include <taglib/tpicturetype.h>
 
 // Replaces the typemap from swigtype.swg and just adds the line
 // SWIG_RubyUnlinkObjects. This is done to be safe in the case when a
@@ -24,6 +25,7 @@
 #include <taglib/tbytevector.h>
 #include <taglib/tbytevectorlist.h>
 #include <taglib/tfile.h>
+#include <taglib/tvariant.h>
 
 #if defined(HAVE_RUBY_ENCODING_H) && HAVE_RUBY_ENCODING_H
 # include <ruby/encoding.h>
@@ -37,34 +39,26 @@
 #endif
 
 VALUE taglib_bytevector_to_ruby_string(const TagLib::ByteVector &byteVector) {
-  if (byteVector.isNull()) {
-    return Qnil;
-  } else {
-    return rb_str_new(byteVector.data(), byteVector.size());
-  }
+  return rb_str_new(byteVector.data(), byteVector.size());
 }
 
 TagLib::ByteVector ruby_string_to_taglib_bytevector(VALUE s) {
   if (NIL_P(s)) {
-    return TagLib::ByteVector::null;
+    return TagLib::ByteVector();
   } else {
     return TagLib::ByteVector(RSTRING_PTR(StringValue(s)), RSTRING_LEN(s));
   }
 }
 
 VALUE taglib_string_to_ruby_string(const TagLib::String & string) {
-  if (string.isNull()) {
-    return Qnil;
-  } else {
-    VALUE result = rb_str_new2(string.toCString(true));
-    ASSOCIATE_UTF8_ENCODING(result);
-    return result;
-  }
+  VALUE result = rb_str_new2(string.toCString(true));
+  ASSOCIATE_UTF8_ENCODING(result);
+  return result;
 }
 
 TagLib::String ruby_string_to_taglib_string(VALUE s) {
   if (NIL_P(s)) {
-    return TagLib::String::null;
+    return TagLib::String();
   } else {
     return TagLib::String(RSTRING_PTR(CONVERT_TO_UTF8(StringValue(s))), TagLib::String::UTF8);
   }
diff --git a/ext/taglib_base/taglib_base.i b/ext/taglib_base/taglib_base.i
index 7ad38dd..08878fc 100644
--- a/ext/taglib_base/taglib_base.i
+++ b/ext/taglib_base/taglib_base.i
@@ -6,6 +6,10 @@
 #include <taglib/tlist.h>
 #include <taglib/fileref.h>
 #include <taglib/tag.h>
+#include <taglib/tpropertymap.h>
+#include <taglib/tvariant.h>
+#include <taglib/tpicturetype.h>
+using namespace TagLib;
 %}
 
 %include "includes.i"
diff --git a/ext/taglib_flac/taglib_flac.i b/ext/taglib_flac/taglib_flac.i
index 2dded03..8e3c6f0 100644
--- a/ext/taglib_flac/taglib_flac.i
+++ b/ext/taglib_flac/taglib_flac.i
@@ -5,6 +5,7 @@
 #include <taglib/flacproperties.h>
 #include <taglib/id3v1tag.h>
 #include <taglib/id3v2tag.h>
+using namespace TagLib;
 %}
 
 %include "../taglib_base/includes.i"
diff --git a/ext/taglib_id3v1/taglib_id3v1.i b/ext/taglib_id3v1/taglib_id3v1.i
index c1cf49f..f751f22 100644
--- a/ext/taglib_id3v1/taglib_id3v1.i
+++ b/ext/taglib_id3v1/taglib_id3v1.i
@@ -2,6 +2,8 @@
 %{
 #include <taglib/id3v1tag.h>
 #include <taglib/id3v1genres.h>
+#include <taglib/tpicturetype.h>
+using namespace TagLib;
 %}
 
 %include "../taglib_base/includes.i"
diff --git a/ext/taglib_id3v2/taglib_id3v2.i b/ext/taglib_id3v2/taglib_id3v2.i
index 07f3e3c..2ed4e6c 100644
--- a/ext/taglib_id3v2/taglib_id3v2.i
+++ b/ext/taglib_id3v2/taglib_id3v2.i
@@ -20,6 +20,7 @@
 #include <taglib/unknownframe.h>
 #include <taglib/unsynchronizedlyricsframe.h>
 #include <taglib/urllinkframe.h>
+using namespace TagLib;
 %}
 
 %include "../taglib_base/includes.i"
@@ -81,6 +82,7 @@ VALUE taglib_id3v2_framelist_to_ruby_array(TagLib::ID3v2::FrameList *list) {
 }
 %}
 
+%include <taglib/id3v2.h>
 %include <taglib/id3v2header.h>
 
 // Only useful internally.
diff --git a/ext/taglib_mp4/taglib_mp4.i b/ext/taglib_mp4/taglib_mp4.i
index be1c9fe..0092177 100644
--- a/ext/taglib_mp4/taglib_mp4.i
+++ b/ext/taglib_mp4/taglib_mp4.i
@@ -5,8 +5,10 @@
 #include <taglib/mp4properties.h>
 #include <taglib/mp4tag.h>
 #include <taglib/mp4atom.h>
+#include <taglib/tpicturetype.h>
 // To resolve some symbols, like AtomDataType in Item.
 using namespace TagLib::MP4;
+using namespace TagLib;
 %}
 
 %include "../taglib_base/includes.i"
diff --git a/ext/taglib_mpeg/taglib_mpeg.i b/ext/taglib_mpeg/taglib_mpeg.i
index 51a75ac..8509f06 100644
--- a/ext/taglib_mpeg/taglib_mpeg.i
+++ b/ext/taglib_mpeg/taglib_mpeg.i
@@ -6,6 +6,8 @@
 #include <taglib/mpegproperties.h>
 #include <taglib/mpegfile.h>
 #include <taglib/id3v2tag.h>
+#include <taglib/tpicturetype.h>
+using namespace TagLib;
 %}
 
 %include "../taglib_base/includes.i"
diff --git a/ext/taglib_wav/taglib_wav.i b/ext/taglib_wav/taglib_wav.i
index 76b8434..2ca3afe 100644
--- a/ext/taglib_wav/taglib_wav.i
+++ b/ext/taglib_wav/taglib_wav.i
@@ -4,7 +4,10 @@
 #include <taglib/wavfile.h>
 #include <taglib/wavproperties.h>
 #include <taglib/id3v2tag.h>
+#include <taglib/tpicturetype.h>
 using namespace TagLib::RIFF;
+using namespace TagLib;
+using StripTags = TagLib::File::StripTags;
 %}
 
 %include "../taglib_base/includes.i"
@@ -55,7 +58,11 @@ namespace TagLib {
   static void free_taglib_riff_wav_file(void *ptr) {
     TagLib::RIFF::WAV::File *file = (TagLib::RIFF::WAV::File *) ptr;
 
+#if TAGLIB_MAJOR_VERSION >= 2
+    TagLib::ID3v2::Tag *id3v2tag = file->ID3v2Tag();
+#else
     TagLib::ID3v2::Tag *id3v2tag = file->tag();
+#endif
     if (id3v2tag) {
       TagLib::ID3v2::FrameList frames = id3v2tag->frameList();
       for (TagLib::ID3v2::FrameList::ConstIterator it = frames.begin(); it != frames.end(); it++) {
diff --git a/tasks/ext.rake b/tasks/ext.rake
index b86eaef..02db67e 100644
--- a/tasks/ext.rake
+++ b/tasks/ext.rake
@@ -20,6 +20,7 @@ taglib_url = "https://github.com/taglib/taglib/archive/v#{taglib_version}.tar.gz
 taglib_options = ['-DCMAKE_BUILD_TYPE=Release',
                   '-DBUILD_EXAMPLES=OFF',
                   '-DBUILD_TESTS=OFF',
+                  '-DBUILD_TESTING=OFF', # used since 1.13 instead of BUILD_TESTS
                   '-DBUILD_BINDINGS=OFF', # 1.11 builds bindings by default
                   '-DBUILD_SHARED_LIBS=ON', # 1.11 builds static by default
                   '-DWITH_MP4=ON', # WITH_MP4, WITH_ASF only needed with taglib 1.7, will be default in 1.8
diff --git a/test/id3v2_write_test.rb b/test/id3v2_write_test.rb
index bc14e89..e093640 100644
--- a/test/id3v2_write_test.rb
+++ b/test/id3v2_write_test.rb
@@ -54,7 +54,7 @@ class TestID3v2Write < Test::Unit::TestCase
     end
 
     should 'be able to save ID3v2.3' do
-      success = @file.save(TagLib::MPEG::File::ID3v2, true, 3)
+      success = @file.save(TagLib::MPEG::File::ID3v2, TagLib::File::StripOthers, TagLib::ID3v2::V3)
       assert_equal true, success
       @file.close
       @file = nil
diff --git a/test/wav_examples_test.rb b/test/wav_examples_test.rb
index 4c982bd..3b0a3fc 100644
--- a/test/wav_examples_test.rb
+++ b/test/wav_examples_test.rb
@@ -19,7 +19,7 @@ class WAVExamples < Test::Unit::TestCase
 
       # Saving ID3v2 cover-art to disk
       TagLib::RIFF::WAV::File.open("#{DATA_FILE_PREFIX}sample.wav") do |file|
-        id3v2_tag = file.tag
+        id3v2_tag = file.id3v2_tag
         cover = id3v2_tag.frame_list('APIC').first
         ext = cover.mime_type.rpartition('/')[2]
         File.open("#{DATA_FILE_PREFIX}cover-art.#{ext}", 'wb') { |f| f.write cover.picture }
diff --git a/test/wav_file_test.rb b/test/wav_file_test.rb
index 9368ac4..4082dcc 100644
--- a/test/wav_file_test.rb
+++ b/test/wav_file_test.rb
@@ -40,7 +40,7 @@ class WAVFileTest < Test::Unit::TestCase
 
     context 'ID3V2 tag' do
       setup do
-        @tag = @file.tag
+        @tag = @file.id3v2_tag
       end
 
       should 'exist' do
diff --git a/test/wav_file_write_test.rb b/test/wav_file_write_test.rb
index ac61d0e..c0d2865 100644
--- a/test/wav_file_write_test.rb
+++ b/test/wav_file_write_test.rb
@@ -33,18 +33,18 @@ class WAVFileWriteTest < Test::Unit::TestCase
     end
 
     should 'have one picture frame' do
-      assert_equal 2, @file.tag.frame_list('APIC').size
+      assert_equal 2, @file.id3v2_tag.frame_list('APIC').size
     end
 
     should 'be able to remove all picture frames' do
-      @file.tag.remove_frames('APIC')
+      @file.id3v2_tag.remove_frames('APIC')
       success = @file.save
       assert success
       @file.close
       @file = nil
 
       reloaded do |file|
-        assert_equal 0, file.tag.frame_list('APIC').size
+        assert_equal 0, file.id3v2_tag.frame_list('APIC').size
       end
     end
 
@@ -58,18 +58,18 @@ class WAVFileWriteTest < Test::Unit::TestCase
       apic.picture = picture_data
       apic.type = TagLib::ID3v2::AttachedPictureFrame::BackCover
 
-      @file.tag.add_frame(apic)
+      @file.id3v2_tag.add_frame(apic)
       success = @file.save
       assert success
       @file.close
       @file = nil
 
       reloaded do |file|
-        assert_equal 3, file.tag.frame_list('APIC').size
+        assert_equal 3, file.id3v2_tag.frame_list('APIC').size
       end
 
       reloaded do |file|
-        written_apic = file.tag.frame_list('APIC')[2]
+        written_apic = file.id3v2_tag.frame_list('APIC')[2]
         assert_equal 'image/jpeg', written_apic.mime_type
         assert_equal 'desc', written_apic.description
         assert_equal picture_data, written_apic.picture

[jacobvosmaer]

@ufleisch thank you so much for spending time on this!

In the way how taglib-ruby is built in the Github workflow, it is linked to the system TagLib, and not to the TagLib 2.0 in the TAGLIB_DIR. Can this be fixed?

It looks like we always use --with-opt-dir= in extconf_common.rb, even if TAGLIB_DIR is set. @robinst Maybe we should only set --with-opt-dir if there is no $TAGLIB_DIR? Similarly for the GEM_HOME vendor guessing. The moment TAGLIB_DIR is set we should not be guessing at all.

[robinst]

Great work!

I patched TagLib 2.0 like this.

Hm I'd really like to avoid having to patch TagLib to run swig on it. Have you tried with bumping the swig version to newest?

Other than that, we should look into whether there's ways to ignore/override whatever the problem is in the swig files.

[ufleisch]

Hm I'd really like to avoid having to patch TagLib to run swig on it. Have you tried with bumping the swig version to newest?

Yes, the reason why I try to build taglib-ruby is that I want to make sure that there will be no problems with building taglib-ruby when TagLib 2.0.1 is released. I have now created a PR taglib#1220 which fixes the issues with MP4::Item and ~StreamTypeResolver(). I have then replaced the TagLib sources with the sources of that PR.

mkdir -p tmp && cd tmp && rm -rf taglib-2.0* && \
curl -sL https://github.com/ufleisch/taglib/archive/refs/heads/swig.tar.gz | tar xzf - && \
mv taglib-swig taglib-2.0 && tar czf taglib-2.0.tar.gz taglib-2.0 && cd -

It builds, but the same tests still fail (MP4 item_map and TagLib::ID3v2::Version). How can this be fixed?

[jacobvosmaer]

I can take a look at the MP4 itemmap code. It's been a long time but I originally contributed most of it to taglib-ruby.

[jacobvosmaer]

@ufleisch I'm trying to run Swig 4.1.1 on your swig branch but it fails with a syntax error.

/Users/jacobvosmaer/tmp/taglib/include/taglib/attachedpictureframe.h:61: Error: Syntax error in input(3).
rake aborted!
Command failed with status (1): [cd ext/taglib_id3v2 && /opt/local/bin/swig -c++ -ruby -autorename -initname taglib_id3v2 -I/Users/jacobvosmaer/tmp/taglib/include -I/usr/local/include -I/usr/include taglib_id3v2.i]

The error points to this line. I don't see what is wrong with it, do you?

[jacobvosmaer]

Oh wait maybe this has to do with the #include <taglib/tpicturetype.h> lines you added?

Edit: yes it did. I applied the big diff and the swig error went away.

[jacobvosmaer]

Now swig ran but I still can't compile. Looks like I am getting errors complaining about the use of C++11 features. For example:

/Users/jacobvosmaer/tmp/taglib/include/taglib/tbytevector.h:52:34: warning: alias declarations are a C++11 extension [-Wc++11-extensions]
    using ConstReverseIterator = std::vector<char>::const_reverse_iterator;
                                 ^
/Users/jacobvosmaer/tmp/taglib/include/taglib/tbytevector.h:627:29: error: expected ';' at end of declaration list
    void swap(ByteVector &v) noexcept;
                            ^
                            ;

Note the warning about C++11. I looked up noexcept and it seems to not exist prior to C++11. I'm trying things like this in ext/extconf_common.rb but with no luck so far.

$CXXFLAGS << '-std=c++17'

[jacobvosmaer]

OK, $CXXFLAGS += ' -std=c++17' does work, I just had to rm -rf tmp to get new mkmf Makefiles.

[ufleisch]

I did not have to add this, but that is probably because I am working on Linux and you are on macOS, right?

[jacobvosmaer]

Yes adding -std=c++17 feels like something that has to do with compiler defaults to me.

Looking into the MP4 test failures, I noticed something interesting. Swig is creating two types for MP4::ItemMap (_swigc__p_ItemMap and _swigc__p_TagLib__MapT_TagLib__String_TagLib__MP4__Item_t), and all the extra stuff we define is on the latter type, but Tag.item_map returns the former type.

Edit: wonder if this has to do with taglib/taglib@c083d7ce1583459382891e5827ca8b8a759ece80.

[ufleisch]

It was probably that commit which changed MP4 stuff. Note that in TagLib 1.x MP4::ItemMap was defined in mp4tag.h, but in TagLib 2.x it is defined in mp4item.h. Maybe the sequence of including a library header and defining SWIG interface stuff depending on it is important and has to be changed now.

[jacobvosmaer]

I got the MP4 stuff working in https://github.com/robinst/taglib-ruby/commit/1408fc981ba4e0439b814bbb8c8fc155d88258db#diff-2ac8d6ce3aefbdd31fbca5ea26476803dbe3b2ed7bc4e41e391f54014fb25117

It's still a mess but I wanted to show what I did.

• changed order of %include <taglib/mp4item.h> and %include <taglib/mp4tag.h>
• added an ignore for another File constructor that uses IOStream
• some other stuff??

It swigs, compiles, and passes mp4 tests is all I can say at the moment.

[ufleisch]

Great! Now I have only the thing with the TagLib::ID3v2::Version as a failing test.

[jacobvosmaer]

I fixed the ID3V2 save test with 7b52873.

[jacobvosmaer]

It appears Swig is very sensitive to the order in which you %include things. In the ID3V2 case there was an enum in id3v2.h for the version constants and Swig did not "understand" that that was just an int because it had not seen id3v2.h yet. The generated code in the save function was type-checking and looking for that unknown Version class. By including id3v2.h earlier, Swift knows that Version is just an int and it generates different wrapper code.

[ufleisch]

Thanks a lot! Then I will plan to release TagLib 2.0.1 soon. If you find things to fix before the patch release, please let me know.

[jacobvosmaer]

@robinst my branch https://github.com/robinst/taglib-ruby/tree/jv-taglib-2-mp4 is not in a clean shape but it swigs, compiles and passes the test against @ufleisch 's swig branch. What do you think about https://github.com/robinst/taglib-ruby/pull/129#issuecomment-2016868321?

[robinst]

Sounds great! I hadn't realised we have a maintainer of TagLib here, awesome :).