-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
libid3tag: Use tenacityteam fork #26140
base: master
Are you sure you want to change the base?
Conversation
Thanks! Duplicates #23259, though. |
self.cpp_info.defines.append("ID3TAG_EXPORT=" + ("__declspec(dllimport)" if self.options.shared else "")) | ||
else: | ||
self.cpp_info.libs = ["id3tag"] | ||
self.cpp_info.libs = ["id3tag"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.cpp_info.libs = ["id3tag"] | |
self.cpp_info.set_property("cmake_file_name", "id3tag") | |
self.cpp_info.set_property("cmake_target_name", "id3tag::id3tag") | |
self.cpp_info.set_property("pkg_config_name", "id3tag") | |
self.cpp_info.libs = ["id3tag"] |
I would recommend including this info even if it matches the defaults. It makes it clear that these are the actual official target names and not just implicit values generated by Conan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the actual package name is libid3tag
, so these would be necessary if the upstream are defining the targets as id3tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed that part, since I copied these from my older PR.
Either way, these are valid:
- https://codeberg.org/tenacityteam/libid3tag/src/branch/main/packaging
- https://packages.debian.org/trixie/amd64/libid3tag0-dev/filelist
Please do double check these details when reviewing, as it's going to be a breaking change for the consumers or will at least needs a "backwards compatible" fix when corrected later.
self.options.rm_safe("fPIC") | ||
self.settings.rm_safe("compiler.libcxx") | ||
self.settings.rm_safe("compiler.cppstd") | ||
languages = ["C"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Would be great if this could be added to the package templates as well as the preferred alternative to self.settings.rm_safe("compiler.libcxx"); self.settings.rm_safe("compiler.cppstd")
.
self.cpp_info.defines.append("ID3TAG_EXPORT=" + ("__declspec(dllimport)" if self.options.shared else "")) | ||
else: | ||
self.cpp_info.libs = ["id3tag"] | ||
self.cpp_info.libs = ["id3tag"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the actual package name is libid3tag
, so these would be necessary if the upstream are defining the targets as id3tag
@@ -15,6 +15,12 @@ def requirements(self): | |||
def layout(self): | |||
cmake_layout(self) | |||
|
|||
def generate(self): | |||
tc = CMakeToolchain(self) | |||
if self.settings.os == "Windows" and self.dependencies[self.tested_reference_str].options.shared: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the complexity of the test, specially when this doesn't seem something that the recipe is managing.
Co-authored-by: Martin Valgur <martin.valgur@gmail.com>
Summary
Changes to recipe: libid3tag/[*]
Motivation
The original repository for libid3tag is no longer (and has been for a while) the preferred way to use the library, opting instead for https://codeberg.org/tenacityteam/libid3tag, which as of right now is even ABI compatible with the latest version.
This fork usage simplifies the recipe and maintenance by a lot, while also providing fixes for the old versions, such as Macos crossbuild support on shared libraries.
Audiowaveform is the only CCI recipe using the old version, we should think about bumping it too
Details
This fork is used by many, such as:
So this PR only brings CCI inline with the rest of the ecosystem for this library
The main consideration for this PR is the test changes, which are explained in the comment for the test_package under Windows&shared
Successful crossbuilding on Macos shared, which previously was disabled due to compilation issues: