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

Add EXIV2_ENABLE_FILESYSTEM_ACCESS option #2837

Merged
merged 2 commits into from
Dec 2, 2023

Conversation

enen92
Copy link
Contributor

@enen92 enen92 commented Nov 17, 2023

Hey,

I am trying to add exiv2 as a dependency for Kodi/XBMC as it seems the most complete GPL2 library for image metadata extraction (since it covers both EXIF and IPTC which we currently do with custom parsers - pretty much untouched since the original xbox days ~ 14 years ago). As you might easily guess, it's pretty much broken by now :)

Unfortunately, since the application is available on multiple platforms some of which are not supported by default by exiv2 (e.g. android, tvos, ios, xbox/uwp, etc) filesystem access is not something that can be taken for granted. The usage we plan to do of the library only requires that we are able to read the image (for tag extraction) from a memory buffer - hence we only really need the MemIO.

https://github.com/xbmc/xbmc/pull/24109/files#diff-40b93a8f1858865324006f152285b7b28de2a1b8e878cb9e04ac4f4f4d11dd0fR35-R36

PoC for inclusion is being done in xbmc/xbmc#24109

This PR proposes an additional option EXIV2_ENABLE_FILESYSTEM_ACCESS to allow disable FileIO and generic filesystem access. This is only valid if unit tests, the exiv app, fuzzer, etc are not enable so I think it shouldn't affect the normal behaviour of the application.
In case this is accepted probably the CI should have a way to validate further builds.

@ghost
Copy link

ghost commented Nov 17, 2023

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

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@enen92 enen92 force-pushed the optional_filesystem branch 2 times, most recently from 609e973 to 42ffad5 Compare November 17, 2023 19:13
@enen92 enen92 marked this pull request as ready for review November 17, 2023 22:06
@enen92 enen92 force-pushed the optional_filesystem branch 2 times, most recently from 71e9227 to 23747ef Compare November 19, 2023 12:53
src/makernote_int.cpp Outdated Show resolved Hide resolved
@enen92 enen92 force-pushed the optional_filesystem branch from 23747ef to 28cada7 Compare November 20, 2023 12:34
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (9f90144) 63.88% compared to head (96f9e9a) 63.88%.

Files Patch % Lines
src/basicio.cpp 0.00% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2837   +/-   ##
=======================================
  Coverage   63.88%   63.88%           
=======================================
  Files         103      103           
  Lines       22369    22369           
  Branches    10865    10865           
=======================================
  Hits        14291    14291           
  Misses       5856     5856           
  Partials     2222     2222           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kmilos kmilos added the platforms Cygwin, MinGW, cross-compilation, NetBSD, FreeBSD etc label Nov 20, 2023
@enen92 enen92 force-pushed the optional_filesystem branch from 28cada7 to 08cd06f Compare November 20, 2023 23:41
@enen92
Copy link
Contributor Author

enen92 commented Nov 20, 2023

Added a new commit to add a new job to validate a build with filesystem disabled. This likely won't run as part of this PR but you can check the result here: https://github.com/enen92/exiv2/actions/runs/6937027229/job/18870288972?pr=2

@enen92
Copy link
Contributor Author

enen92 commented Nov 28, 2023

rebased after #2844 got merged

@enen92 enen92 force-pushed the optional_filesystem branch from 7ba16b6 to bed843d Compare November 28, 2023 22:01
src/basicio.cpp Outdated Show resolved Hide resolved
src/image.cpp Show resolved Hide resolved
src/futils.cpp Show resolved Hide resolved
src/futils.cpp Show resolved Hide resolved
@enen92 enen92 force-pushed the optional_filesystem branch from bed843d to 50d5ce3 Compare November 30, 2023 23:11
@enen92 enen92 force-pushed the optional_filesystem branch from 50d5ce3 to 96f9e9a Compare November 30, 2023 23:14
@neheb neheb merged commit 582664c into Exiv2:main Dec 2, 2023
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platforms Cygwin, MinGW, cross-compilation, NetBSD, FreeBSD etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants