Skip to content

Add conf-taglib_c #28001

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

Merged
merged 1 commit into from
Jul 10, 2025
Merged

Add conf-taglib_c #28001

merged 1 commit into from
Jul 10, 2025

Conversation

EruEri
Copy link
Contributor

@EruEri EruEri commented Jun 8, 2025

Run pkg-config to check if this library is installed on the system.

  • conf-taglib_c.1 checks if a minimal version of 1.0.0
  • conf-taglib_c.2 checks if a minimal version of 2.0.0

But I have some questions.

How should we handle depexts for distributions providing the package but not with a required version (.eg fedora provides taglib-devel but currently only at the version 1.13.1), should we list them in the depexts field or not ?

Also taglib is double-licensed under the LGPL-2.1 and MPL-1.1 but according to mozilla, the MPL-1.1 can be upgraded to MPL-2.0. So in the license field, can I put MPL-2.0 instead of MPL-1.1 or should we stick to the MPL-1.1 or the links from mozilla is irrelevant ?

Thanks for your time.

@mseri
Copy link
Member

mseri commented Jun 9, 2025

According to what is written there, you can

depends: [
"conf-pkg-config" {build}
]
depexts: [
Copy link
Member

Choose a reason for hiding this comment

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

Why there are less depexts than above?
I think libtag-c-dev should be there for ubumtu for example: https://packages.ubuntu.com/plucky/libtag-c-dev

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it not an issue that the package libtag-c-dev only seems to exist in the latest version of ubuntu (25.04) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I didn't know that the latest version of ubuntu had this version.
I removed fedora since the taglib package https://packages.fedoraproject.org/pkgs/taglib/taglib-devel/ doesn't provide the required version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or should I still put the package ?

Copy link
Member

Choose a reason for hiding this comment

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

The fact that is only in a recent version is not a problem, there is a way to filter the package name by os version in those cases: https://opam.ocaml.org/doc/Manual.html#opamvar-os-version

Copy link
Contributor Author

@EruEri EruEri Jun 15, 2025

Choose a reason for hiding this comment

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

So for packages not yet up to date into distribution repos, should I filter using the opam variable os-version ?

Copy link
Member

Choose a reason for hiding this comment

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

If there is a choice of name to be made, I'd say yes

@Zeta611
Copy link
Member

Zeta611 commented Jul 10, 2025

@EruEri
Copy link
Contributor Author

EruEri commented Jul 10, 2025

Is the CI failure conf-taglib_c.2 (failed: The compilation of conf-taglib_c.2 failed at "pkg-config --atleast-version 2.0.0 --exists taglib_c".) expected for most of the Linux distributions?

I think so, since most of the stable one haven't upgraded this package to the new major release.

  • Fedora 1.13.1
  • Ubuntu prior to 25.04, it'is not the the new major version
  • Alpine linux has a good version since v3.22, the version v3.21 of alpine has the 1.13 version of the library
    ...

I could also check for debian and opensuse

Copy link
Member

@mseri mseri left a comment

Choose a reason for hiding this comment

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

I think within the constraints of the depext system, this is doing the best we can. Let's merge. Thanks for the patience

@mseri mseri merged commit 5096cfa into ocaml:master Jul 10, 2025
1 of 3 checks passed
@EruEri
Copy link
Contributor Author

EruEri commented Jul 10, 2025

I think within the constraints of the depext system, this is doing the best we can. Let's merge. Thanks for the patience

Thanks to you for your feedback and your answer to my questions

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.

4 participants