Add a cmake config file export #2703

Ryanf55 · 2023-07-18T15:13:54Z

This is an example of the path forward for #2687 . More work will be required for all the dependencies, and propagating the build options such as EXIV2_ENABLE_PNG to add/remove find_dependency calls.

Edit: All actions are complete in scope of the original issue.

Note: This work was done on behalf of AeroVironment Inc.

ghost · 2023-07-18T15:15:13Z

👇 Click on the image for a new way to code review

Legend

Ryanf55 · 2023-07-18T15:20:27Z

cmake/exiv2Config.cmake.in

+cmake_minimum_required(VERSION 3.16.3)
+include(CMakeFindDependencyMacro)
+
+find_dependency(ZLIB REQUIRED)


This is the find dependency call I mentioned that was missing. This should only be added to the generated file if it was built with PNG support.

Here is one approach:
https://discourse.cmake.org/t/installing-exporting-projects-that-conditionally-use-optional-dependencies/5194/2

I used this approach. With EXIV2_ENABLE_PNG turned on, the following config file gets generated:

cmake_minimum_required(VERSION 3.16.3) include(CMakeFindDependencyMacro) if(ON) # if(EXIV2_ENABLE_PNG) find_dependency(ZLIB REQUIRED) endif() include("${CMAKE_CURRENT_LIST_DIR}/exiv2Export.cmake") check_required_components(exiv2)

Ryanf55 · 2023-07-18T15:28:46Z

src/CMakeLists.txt

-    RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
-    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
-    ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
+install(TARGETS exiv2lib EXPORT exiv2Export)


In CMake 3.14 and above, you don't need to supply the RUNTIME, LIBRARY, and ARCHIVE directories. This relies on the defaults, which conveniently were the same as before.

As CMake was reduced to 3.5, is this still relevant?

Yup! This cmake version required to build exiv2 is 3.16.3 which is when this line of code runs. This version is unchanged in this PR.

The version required to consume exiv2 is now 3.5. Previously, no such minimum version was specified, it was undefined.

They are independent.

src/CMakeLists.txt

codecov · 2023-07-18T15:58:08Z

Codecov Report

Merging #2703 (2d3b05c) into main (c827648) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2703   +/-   ##
=======================================
  Coverage   63.91%   63.91%           
=======================================
  Files         103      103           
  Lines       22313    22313           
  Branches    10804    10804           
=======================================
  Hits        14261    14261           
  Misses       5828     5828           
  Partials     2224     2224

neheb · 2023-07-18T18:01:43Z

I'll just mention in passing that a recent big endian bug that got fixed I tested on cfarm's GCC230, which has

~$ cmake --version
cmake version 3.13.4

although buster-backports has 3.18.4 , so probably better to update to that.

Ryanf55 · 2023-07-18T18:29:28Z

I'll just mention in passing that a recent big endian bug that got fixed I tested on cfarm's GCC230, which has
~$ cmake --version
cmake version 3.13.4
although buster-backports has 3.18.4 , so probably better to update to that.

You are right, I could probably downgrade the installation version of cmake, probably to 3.5. None of the logic in there is new.

Ryanf55 · 2023-07-19T15:32:22Z

Is this PR in the right direction you were hoping? If so, aside from the part about making find_dependency conditionally show up, what would you need to get this merged? I'm maintaining a patch right now for it, and I'd love to get this upstreamed.

neheb · 2023-07-19T17:23:38Z

Sure. This is still a draft.

Signed-off-by: Ryan Friedman <[email protected]>

* This matches an alias target name * Recommend using the namespaced target in the README Signed-off-by: Ryan Friedman <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]>

Ryanf55 · 2023-07-25T16:12:57Z

I'll just mention in passing that a recent big endian bug that got fixed I tested on cfarm's GCC230, which has
~$ cmake --version
cmake version 3.13.4
although buster-backports has 3.18.4 , so probably better to update to that.
You are right, I could probably downgrade the installation version of cmake, probably to 3.5. None of the logic in there is new.

Downgraded to 3.5 in 2d3b05c

neheb · 2023-07-26T02:54:44Z

Should this be backported?

Ryanf55 · 2023-07-26T03:03:00Z

Should this be backported?

Yes please, it's affecting us on the last release and we are maintaining a patch in exiv2 internally.

I did my best to ensure it's backwards compatible.

neheb · 2023-07-26T03:45:34Z

@Mergifyio backport 0.28.x

mergify · 2023-07-26T03:45:42Z

backport 0.28.x

✅ Backports have been created

#2723 Add a cmake config file export (backport #2703) has been created for branch 0.28.x

neheb · 2023-07-29T00:00:43Z

this PR seems to have caused Windows CI to fail:

 FAILED: bin/exiv2.dll lib/exiv2.lib 
cmd.exe /C "cmd.exe /C ""C:\Program Files\CMake\bin\cmake.exe" -E __create_def D:\a\exiv2\exiv2\build\src\CMakeFiles\exiv2lib.dir\.\exports.def D:\a\exiv2\exiv2\build\src\CMakeFiles\exiv2lib.dir\.\exports.def.objs && cd D:\a\exiv2\exiv2\build" && "C:\Program Files\CMake\bin\cmake.exe" -E vs_link_dll --intdir=src\CMakeFiles\exiv2lib.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1435~1.322\bin\HostX64\x64\link.exe /nologo @CMakeFiles\exiv2lib.rsp  /out:bin\exiv2.dll /implib:lib\exiv2.lib /pdb:src\exiv2.pdb /dll /version:1.0 /machine:x64 /WX /INCREMENTAL:NO /ignore:4099  /DEF:src\CMakeFiles\exiv2lib.dir\.\exports.def  && cd ."
LINK: command "C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1435~1.322\bin\HostX64\x64\link.exe /nologo @CMakeFiles\exiv2lib.rsp /out:bin\exiv2.dll /implib:lib\exiv2.lib /pdb:src\exiv2.pdb /dll /version:1.0 /machine:x64 /WX /INCREMENTAL:NO /ignore:4099 /DEF:src\CMakeFiles\exiv2lib.dir\.\exports.def /MANIFEST:EMBED,ID=2" failed (exit code 4197) with the following output:
canonmn_int.cpp.obj : warning LNK4197: export '??_7Value@Exiv2@@6B@' specified multiple times; using first specification
LINK : error LNK1218: warning treated as error; no output file generated

Interestingly enough, the CI here didn't catch it but the one on push did.

Ryanf55 · 2023-07-29T00:02:11Z

It's been brought to my attention this somehow broke the windows build due to a linker error.
#2712

https://github.com/Exiv2/exiv2/actions/runs/5695647884/job/15440304541?pr=2712
FAILED: bin/exiv2.dll lib/exiv2.lib 
cmd.exe /C "cmd.exe /C ""C:\Program Files\CMake\bin\cmake.exe" -E __create_def D:\a\exiv2\exiv2\build\src\CMakeFiles\exiv2lib.dir\.\exports.def D:\a\exiv2\exiv2\build\src\CMakeFiles\exiv2lib.dir\.\exports.def.objs && cd D:\a\exiv2\exiv2\build" && "C:\Program Files\CMake\bin\cmake.exe" -E vs_link_dll --intdir=src\CMakeFiles\exiv2lib.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1435~1.322\bin\HostX64\x64\link.exe /nologo @CMakeFiles\exiv2lib.rsp  /out:bin\exiv2.dll /implib:lib\exiv2.lib /pdb:src\exiv2.pdb /dll /version:1.0 /machine:x64 /WX /INCREMENTAL:NO /ignore:4099  /DEF:src\CMakeFiles\exiv2lib.dir\.\exports.def  && cd ."
LINK: command "C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1435~1.322\bin\HostX64\x64\link.exe /nologo @CMakeFiles\exiv2lib.rsp /out:bin\exiv2.dll /implib:lib\exiv2.lib /pdb:src\exiv2.pdb /dll /version:1.0 /machine:x64 /WX /INCREMENTAL:NO /ignore:4099 /DEF:src\CMakeFiles\exiv2lib.dir\.\exports.def /MANIFEST:EMBED,ID=2" failed (exit code 4197) with the following output:
canonmn_int.cpp.obj : warning LNK4197: export '??_7Value@Exiv2@@6B@' specified multiple times; using first specification
LINK : error LNK1218: warning treated as error; no output file generated

What is not clear is how these changes caused a linker failure, AND how they were not caught by CI in this PR. I'll try to set up a Windows computer, but it might take a few days. I would suggest a temporary revert if you are sure the regression was actually caused by this PR.

neheb · 2023-07-29T00:07:12Z

I’m just looking at main, which commits are green and red. The last one is red. No idea what the issue is.

Ryanf55 · 2023-07-29T00:22:59Z

I’m just looking at main, which commits are green and red. The last one is red. No idea what the issue is.

Ok. I have installed Visual Studio on a Windows computer. I'll try to reproduce it locally.

Ryanf55 · 2023-07-29T01:39:45Z

Update - The conan installation and build instructions for Windows don't work, I can't yet compile exiv2.
See #2634

Ryanf55 · 2023-07-29T03:58:24Z

A revert for this PR is added here: #2725
Let's see if CI passes again...

Ryanf55 marked this pull request as draft July 18, 2023 15:14

Ryanf55 commented Jul 18, 2023

View reviewed changes

Ryanf55 force-pushed the fix-cmake-export-config branch from f6a85fb to 793e872 Compare July 18, 2023 15:27

Ryanf55 commented Jul 18, 2023

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

Ryanf55 force-pushed the fix-cmake-export-config branch from 793e872 to 744de24 Compare July 18, 2023 15:36

Ryanf55 added 2 commits July 25, 2023 16:04

Add a cmake config file export

4d48af9

Signed-off-by: Ryan Friedman <[email protected]>

Make ZLIB depend on EXIV2_ENABLE_PNG

d798697

Signed-off-by: Ryan Friedman <[email protected]>

Ryanf55 force-pushed the fix-cmake-export-config branch from 744de24 to d798697 Compare July 25, 2023 16:05

Ryanf55 added 2 commits July 25, 2023 16:11

Add Exiv2 namespace for exported targets

7e25895

* This matches an alias target name * Recommend using the namespaced target in the README Signed-off-by: Ryan Friedman <[email protected]>

Downgrade required CMake version to consume exiv2

2d3b05c

Signed-off-by: Ryan Friedman <[email protected]>

Ryanf55 marked this pull request as ready for review July 25, 2023 16:12

Ryanf55 requested a review from neheb July 26, 2023 02:09

neheb approved these changes Jul 26, 2023

View reviewed changes

neheb merged commit 368eab0 into Exiv2:main Jul 26, 2023

mergify bot mentioned this pull request Jul 26, 2023

Add a cmake config file export (backport #2703) #2723

Closed

Ryanf55 mentioned this pull request Jul 26, 2023

The link interface of target "exiv2lib" contains: ZLIB::ZLIB but the target was not found #2687

Closed

Ryanf55 deleted the fix-cmake-export-config branch July 26, 2023 15:20

neheb mentioned this pull request Jul 28, 2023

meson changes #2712

Merged

kmilos mentioned this pull request Nov 6, 2023

install exiv2 cmake module in LIBDIR #2819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a cmake config file export #2703

Add a cmake config file export #2703

Ryanf55 commented Jul 18, 2023 •

edited

Loading

ghost commented Jul 18, 2023 •

edited by ghost

Loading

Legend

Ryanf55 Jul 18, 2023

Ryanf55 Jul 25, 2023 •

edited

Loading

Ryanf55 Jul 18, 2023

neheb Jul 25, 2023

Ryanf55 Jul 25, 2023 •

edited

Loading

codecov bot commented Jul 18, 2023 •

edited

Loading

neheb commented Jul 18, 2023

Ryanf55 commented Jul 18, 2023

Ryanf55 commented Jul 19, 2023

neheb commented Jul 19, 2023

Ryanf55 commented Jul 25, 2023

neheb commented Jul 26, 2023

Ryanf55 commented Jul 26, 2023 •

edited

Loading

neheb commented Jul 26, 2023

mergify bot commented Jul 26, 2023 •

edited

Loading

neheb commented Jul 29, 2023

Ryanf55 commented Jul 29, 2023

neheb commented Jul 29, 2023

Ryanf55 commented Jul 29, 2023

Ryanf55 commented Jul 29, 2023 •

edited

Loading

Ryanf55 commented Jul 29, 2023

Add a cmake config file export #2703

Add a cmake config file export #2703

Conversation

Ryanf55 commented Jul 18, 2023 • edited Loading

ghost commented Jul 18, 2023 • edited by ghost Loading

Legend

Ryanf55 Jul 18, 2023

Choose a reason for hiding this comment

Ryanf55 Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Ryanf55 Jul 18, 2023

Choose a reason for hiding this comment

neheb Jul 25, 2023

Choose a reason for hiding this comment

Ryanf55 Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 18, 2023 • edited Loading

Codecov Report

neheb commented Jul 18, 2023

Ryanf55 commented Jul 18, 2023

Ryanf55 commented Jul 19, 2023

neheb commented Jul 19, 2023

Ryanf55 commented Jul 25, 2023

neheb commented Jul 26, 2023

Ryanf55 commented Jul 26, 2023 • edited Loading

neheb commented Jul 26, 2023

mergify bot commented Jul 26, 2023 • edited Loading

✅ Backports have been created

neheb commented Jul 29, 2023

Ryanf55 commented Jul 29, 2023

neheb commented Jul 29, 2023

Ryanf55 commented Jul 29, 2023

Ryanf55 commented Jul 29, 2023 • edited Loading

Ryanf55 commented Jul 29, 2023

Ryanf55 commented Jul 18, 2023 •

edited

Loading

ghost commented Jul 18, 2023 •

edited by ghost

Loading

Ryanf55 Jul 25, 2023 •

edited

Loading

Ryanf55 Jul 25, 2023 •

edited

Loading

codecov bot commented Jul 18, 2023 •

edited

Loading

Ryanf55 commented Jul 26, 2023 •

edited

Loading

mergify bot commented Jul 26, 2023 •

edited

Loading

Ryanf55 commented Jul 29, 2023 •

edited

Loading