Remove libinih from codebase and add it as a dependency instead #2443

kevinbackhouse · 2022-12-27T15:03:29Z

ini.cpp and ini.hpp are copied from this repo: https://github.com/benhoyt/inih. Most platforms have a libinih-dev library that we can add as a dependency, so we can remove ini.cpp and ini.hpp from the exiv2 codebase.

On most platforms, it is easy to replace ini.cpp and ini.hpp with a dependency, but some platforms only have a libinih library and not a libinih-devel library, which is insufficient for building exiv2. On those platforms, I had to update the Actions workflows to build inih from source. This is the list of platforms where I had to do that:

Centos Stream
~~MSYS2~~
Cygwin

On several other platforms, I had to upgrade to a newer version to get a sufficiently recent version of libinih. For example, I upgraded Ubuntu from 20.04 to 22.04.

codecov · 2022-12-27T15:22:10Z

Codecov Report

Merging #2443 (ba258cb) into main (13ecfbc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##             main    #2443    +/-   ##
========================================
  Coverage   65.43%   65.44%            
========================================
  Files         118      117     -1     
  Lines       21661    21554   -107     
  Branches    10465    10434    -31     
========================================
- Hits        14174    14106    -68     
+ Misses       5296     5266    -30     
+ Partials     2191     2182     -9

Impacted Files	Coverage Δ
src/makernote_int.cpp	`66.97% <100.00%> (ø)`
src/pngchunk_int.cpp	`74.73% <0.00%> (-0.27%)`	⬇️
src/pentaxmn_int.cpp	`72.34% <0.00%> (ø)`
src/pngimage.cpp	`62.01% <0.00%> (+0.03%)`	⬆️

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

kmilos · 2022-12-27T15:28:08Z

Et voila for MSYS2: msys2/MINGW-packages#14781

kevinbackhouse · 2022-12-27T15:37:00Z

Et voila for MSYS2: msys2/MINGW-packages#14781

Wow, that was fast! I was looking into it and only just found that repo about a minute ago!

kevinbackhouse · 2022-12-27T15:51:17Z

I'll convert this PR to draft until msys2/MINGW-packages#14781 is ready.

The build failure in CIFuzz should resolve itself automatically after this PR is merged, because OSS-Fuzz uses the install script from our main branch: https://github.com/google/oss-fuzz/blob/582e69cebf7a970faf4ffb0c5492e497d860cb12/projects/exiv2/Dockerfile#L19

kmilos · 2022-12-27T16:45:27Z

@kevinbackhouse Merged. It might take a couple of hours/days until the binary package actually trickles down to the repo...

.github/workflows/on_PR_windows_matrix.yml

kevinbackhouse · 2022-12-29T14:52:41Z

@piponazo: do these cmake/conan changes look ok to you?

piponazo

It seems to me that libinih is a strong dependency and we should be more strict in checking that the dependency is properly found.

Apart from that, all the changes look good to me 👏

.github/workflows/on_push_BasicWinLinMac.yml

cmake/findDependencies.cmake

src/CMakeLists.txt

piponazo

LGTM! Thanks ❤️

kmilos · 2023-01-05T12:33:10Z

FWIW: https://cygwin.com/pipermail/cygwin/2023-January/252780.html (couldn't find any other channel for such requests...)

The other option of course is that someone here actually creates a port file and sends it upstream....

kmilos · 2023-01-05T21:23:37Z

There is follow-up re Cygwin: https://cygwin.com/pipermail/cygwin/2023-January/252781.html

Anyone able to test?

hassec · 2023-01-06T08:27:26Z

@kevinbackhouse This seems to have broken the nighlty release build

kevinbackhouse · 2023-01-06T09:33:32Z

@kevinbackhouse This seems to have broken the nighlty release build

@hassec: sorry about that! I'll look into it.

eddiezato · 2023-01-12T05:31:04Z

I build statically with msys2/ucrt64 to get one exe. Now exiv2.exe asks for libINIReader.dll and I can't solve it with -linih or -linireader. Any help, please?

kmilos · 2023-01-12T08:22:26Z

I build statically with msys2/ucrt64 to get one exe. Now exiv2.exe asks for libINIReader.dll

Yep, the MSYS2 package did not ship the static library in the default build options... An update should hit the repo in a couple of hours/days.

Recently, Exiv2 removed some files from their tree to make it a proper dependency: Exiv2/exiv2#2443 Though a comment in this MR seems to say it should be a hard dependency, this other patch says it only disable readExiv2Config() which makes Exiv2 not read custom user config files: Exiv2/exiv2#2465 (also verified by grepping EXV_ENABLE_INIH which indeed shows it is only used by this one function) I assume this is the config file which is discussed of: https://exiv2.org/manpage.html#config_file For GIMP, I am unsure if this custom optional config file is that important and if anyone actually makes use of this. Even more in the flatpak, where eventually the access to $HOME (where the user file would be) is meant to disappear for sandboxing reasons. So instead of adding this additional dep, let's just disable the user config file feature.

Recently, Exiv2 removed some files from their tree to make it a proper dependency: Exiv2/exiv2#2443 Though a comment in this MR seems to say it should be a hard dependency, this other patch says it only disable readExiv2Config() which makes Exiv2 not read custom user config files: Exiv2/exiv2#2465 (also verified by grepping EXV_ENABLE_INIH which indeed shows it is only used by this one function) I assume this is the config file which is discussed of: https://exiv2.org/manpage.html#config_file For GIMP, I am unsure if this custom optional config file is that important and if anyone actually makes use of this. Even more in the flatpak, where eventually the access to $HOME (where the user file would be) is meant to disappear for sandboxing reasons. So instead of adding this additional dep, let's just disable the user config file feature. (cherry picked from commit 4335a09)

Ryanf55 · 2023-07-24T15:31:18Z

Just curious, is there a reason the FindInih.cmake doesn't also check inih_inireader_NOTFOUND?

CMake Error in src/CMakeLists.txt:
  Imported target "inih::inireader" includes non-existent path

    "inih_inireader_INCLUDE_DIR-NOTFOUND"

  in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include:

  * The path was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and references files it does not
  provide.

On alpine, if you install inih-dev without inih-inireader-dev, you get that error.

kmilos · 2023-07-24T15:39:43Z

On alpine, if you install inih-dev without inih-inireader-dev, you get that error.

Granted, this was not envisaged when I updated the module. I briefly checked the "major" distros - these are usually packaged together... For a such a tiny (few KB) library, splitting is more overhead than benefit IMHO...

Ryanf55 · 2023-07-24T15:41:12Z

inih_inireader_NOTFOUND

On alpine, they are separate installs. Seems like a quick one-liner could fix it. I'll make a patch.

* Fixes issue on alpine where inih-dev is installed without inih-inireader-dev * Exiv2#2443 (comment) Signed-off-by: Ryan Friedman <ryan.friedman+github@avinc.com>

* Fixes issue on alpine where inih-dev is installed without inih-inireader-dev * #2443 (comment) Signed-off-by: Ryan Friedman <ryan.friedman+github@avinc.com>

* Fixes issue on alpine where inih-dev is installed without inih-inireader-dev * #2443 (comment) Signed-off-by: Ryan Friedman <ryan.friedman+github@avinc.com> (cherry picked from commit 16c533f)

kevinbackhouse marked this pull request as draft December 27, 2022 15:51

Remove libinih from codebase and add it as a dependency instead.

a4164ce

kevinbackhouse force-pushed the inih-from-library3 branch from 00a0bac to a4164ce Compare December 28, 2022 15:33

kmilos reviewed Dec 28, 2022

View reviewed changes

.github/workflows/on_PR_windows_matrix.yml Outdated Show resolved Hide resolved

Update .github/workflows/on_PR_windows_matrix.yml

cccf265

kevinbackhouse marked this pull request as ready for review December 28, 2022 15:45

kevinbackhouse added the refactoring Cleanup / Simplify code -> more readable / robust label Dec 28, 2022

kevinbackhouse added this to the v1.00 milestone Dec 28, 2022

kevinbackhouse requested a review from piponazo December 29, 2022 14:51

piponazo requested changes Dec 29, 2022

View reviewed changes

.github/workflows/on_push_BasicWinLinMac.yml Show resolved Hide resolved

cmake/findDependencies.cmake Outdated Show resolved Hide resolved

src/CMakeLists.txt Outdated Show resolved Hide resolved

cmake fatal error if inih library is not found.

ba258cb

kevinbackhouse requested a review from piponazo January 2, 2023 16:35

piponazo approved these changes Jan 2, 2023

View reviewed changes

kevinbackhouse merged commit 346e114 into Exiv2:main Jan 4, 2023

kevinbackhouse deleted the inih-from-library3 branch January 4, 2023 10:44

neheb mentioned this pull request Jan 6, 2023

manual fixes #2455

Merged

kevinbackhouse mentioned this pull request Jan 6, 2023

Add inih dependency to release workflow #2457

Merged

nehaljwani mentioned this pull request Apr 30, 2023

Is a new release happening soon? #2406

Closed

Ryanf55 mentioned this pull request Jul 24, 2023

Make FindInih also make finding reader required #2721

Merged

neheb pushed a commit that referenced this pull request Jul 24, 2023

Make FindInih also make finding reader required

16c533f

* Fixes issue on alpine where inih-dev is installed without inih-inireader-dev * #2443 (comment) Signed-off-by: Ryan Friedman <ryan.friedman+github@avinc.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove libinih from codebase and add it as a dependency instead #2443

Remove libinih from codebase and add it as a dependency instead #2443

kevinbackhouse commented Dec 27, 2022 •

edited

Loading

codecov bot commented Dec 27, 2022 •

edited

Loading

kmilos commented Dec 27, 2022

kevinbackhouse commented Dec 27, 2022

kevinbackhouse commented Dec 27, 2022

kmilos commented Dec 27, 2022

kevinbackhouse commented Dec 29, 2022

piponazo left a comment

piponazo left a comment

kmilos commented Jan 5, 2023 •

edited

Loading

kmilos commented Jan 5, 2023

hassec commented Jan 6, 2023

kevinbackhouse commented Jan 6, 2023

eddiezato commented Jan 12, 2023

kmilos commented Jan 12, 2023 •

edited

Loading

Ryanf55 commented Jul 24, 2023

kmilos commented Jul 24, 2023 •

edited

Loading

Ryanf55 commented Jul 24, 2023 •

edited

Loading

Remove libinih from codebase and add it as a dependency instead #2443

Remove libinih from codebase and add it as a dependency instead #2443

Conversation

kevinbackhouse commented Dec 27, 2022 • edited Loading

codecov bot commented Dec 27, 2022 • edited Loading

Codecov Report

kmilos commented Dec 27, 2022

kevinbackhouse commented Dec 27, 2022

kevinbackhouse commented Dec 27, 2022

kmilos commented Dec 27, 2022

kevinbackhouse commented Dec 29, 2022

piponazo left a comment

Choose a reason for hiding this comment

piponazo left a comment

Choose a reason for hiding this comment

kmilos commented Jan 5, 2023 • edited Loading

kmilos commented Jan 5, 2023

hassec commented Jan 6, 2023

kevinbackhouse commented Jan 6, 2023

eddiezato commented Jan 12, 2023

kmilos commented Jan 12, 2023 • edited Loading

Ryanf55 commented Jul 24, 2023

kmilos commented Jul 24, 2023 • edited Loading

Ryanf55 commented Jul 24, 2023 • edited Loading

kevinbackhouse commented Dec 27, 2022 •

edited

Loading

codecov bot commented Dec 27, 2022 •

edited

Loading

kmilos commented Jan 5, 2023 •

edited

Loading

kmilos commented Jan 12, 2023 •

edited

Loading

kmilos commented Jul 24, 2023 •

edited

Loading

Ryanf55 commented Jul 24, 2023 •

edited

Loading