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

Improve pkg-config file generation #2471

Merged
merged 3 commits into from
Jan 27, 2023
Merged

Improve pkg-config file generation #2471

merged 3 commits into from
Jan 27, 2023

Conversation

kmilos
Copy link
Collaborator

@kmilos kmilos commented Jan 25, 2023

No description provided.

@ghost
Copy link

ghost commented Jan 25, 2023

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #2471 (15f0119) into main (a87c59b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2471   +/-   ##
=======================================
  Coverage   62.11%   62.11%           
=======================================
  Files         121      121           
  Lines       22819    22819           
  Branches    11209    11209           
=======================================
  Hits        14173    14173           
  Misses       6439     6439           
  Partials     2207     2207           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cmake/exiv2.pc.in Outdated Show resolved Hide resolved
@kmilos kmilos marked this pull request as draft January 25, 2023 15:20
@kmilos kmilos marked this pull request as ready for review January 25, 2023 21:35
@neheb
Copy link
Collaborator

neheb commented Jan 26, 2023

my meson PR produces

prefix=/usr/local
includedir=${prefix}/include
libdir=${prefix}/lib64

Name: exiv2
Description: Exif/IPTC/Xmp C++ metadata library and tools plus ICC Profiles, Previews and more.
URL: https://exiv2.org
Version: 1.0.0
Requires.private: INIReader, libbrotlidec, libcurl, zlib
Libs: -L${libdir} -lexiv2
Cflags: -I${includedir}

I assume it's similar here?

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 26, 2023

Thanks for comparing and letting me know @neheb!

Indeed, there are further two things I'll need to fix: no need for further dependencies (inih and libbrotlicommon; they'll get pulled in by the respective .pc files), and comma delimited list instead of space delimited.

I'm also surprised I don't see iconv nor expat in your list...

@kmilos kmilos force-pushed the pc_improve branch 2 times, most recently from 511bc43 to 7df3f71 Compare January 26, 2023 10:01
@kmilos
Copy link
Collaborator Author

kmilos commented Jan 26, 2023

I am now getting (on Windows, MSYS2 UCRT64):

prefix=/ucrt64
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Name: exiv2
Description: Exif/IPTC/Xmp C++ metadata library and tools plus ICC Profiles, Previews and more.
Version: 1.0.0.9
URL: https://exiv2.org
Requires.private: INIReader, expat, iconv, libbrotlidec, libcurl, zlib
Libs: -L${libdir} -lexiv2
Libs.private: -lexiv2-xmp -lintl
Cflags: -I${includedir}

@kmilos kmilos force-pushed the pc_improve branch 2 times, most recently from 6057bca to 3764bd4 Compare January 26, 2023 13:55
@neheb
Copy link
Collaborator

neheb commented Jan 26, 2023

Iconv is libc iconv. Meson does not need to add. I’ll check expat.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 26, 2023

Iconv is libc iconv

I though this was for intl only, but now I see also for iconv, will fix that as well...

Also, I think our own exiv2-xmp should be on the Libs.private list, no? (If not using the external XMP SDK.)

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 26, 2023

Luckily we require CMake 3.11 anyway, so I also removed our custom module: https://cmake.org/cmake/help/latest/module/FindIconv.html

@kmilos kmilos force-pushed the pc_improve branch 4 times, most recently from cf87ad2 to 19cd925 Compare January 26, 2023 16:18
@neheb
Copy link
Collaborator

neheb commented Jan 26, 2023

Expat is not implemented in meson as it’s simpler. I might look into this.

piponazo
piponazo previously approved these changes Jan 26, 2023
@neheb
Copy link
Collaborator

neheb commented Jan 26, 2023

Windows:

prefix=/usr/local
includedir=${prefix}/include
libdir=${prefix}/lib

Name: exiv2
Description: Exif/IPTC/Xmp C++ metadata library and tools plus ICC Profiles, Previews and more.
URL: https://exiv2.org
Version: 1.0.0
Requires.private: INIReader, libbrotlidec, libcurl, zlib
Libs: -L${libdir} -lexiv2
Libs.private: -lws2_32 -liconv -lintl
Cflags: -I${includedir}

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 26, 2023

Hm, I have iconv.pc on MSYS2...

@neheb
Copy link
Collaborator

neheb commented Jan 26, 2023

On meson I check for it no matter the OS. Probably non Windows with CMake.

@mergify mergify bot dismissed piponazo’s stale review January 27, 2023 08:30

Pull request has been modified.

@kmilos
Copy link
Collaborator Author

kmilos commented Jan 27, 2023

Yep, MSYS2 was an exception rather than the rule, so converted iconv to Libs.private as well now.

@kmilos kmilos merged commit 0075ff6 into main Jan 27, 2023
@mergify mergify bot deleted the pc_improve branch January 27, 2023 14:33
@eli-schwartz
Copy link
Contributor

Expat is not implemented in meson as it’s simpler. I might look into this.

expat provides a canonical pkg-config file so there's no need for Meson to implement an override.

@neheb
Copy link
Collaborator

neheb commented Jan 30, 2023

no I meant in the PR. I've since implemented it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants