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

Allow building Exiv2 0.27 with C++17 compiler #2743

Open
wants to merge 1 commit into
base: 0.27-maintenance
Choose a base branch
from

Conversation

maksim-petukhov
Copy link

The only thing that is preventing this from happening is using std::auto_ptr. This PR introduces two compile definitions EXV_NO_AUTO_PTR (I looked at BOOST_NO_AUTO_PTR for inspiration) and EXV_NO_AUTO_PTR_MOVE which are defined only when compiling library with C++17 compiler flag. As far as I can tell there are no existing users of this library which are affected by this change: even using C++11 and C++14 compiler flag user will have the same behavior as before this PR, std::auto_ptr will be used. Looks not really nice, I know, but I think it is OK to have this on maintenance branch. I tested this changes locally with MSVC 192, gcc 10.4 and clang from android sdk r25c.

@postscript-dev
Copy link
Collaborator

@maksim-petukhov
The 0.27-maintenance branch is fixed using C++98 and, as far as I know, there are no plans to change that. However recently, Exiv2 released version 0.28.0 which is compatible with C++20.

Hopefully the new release provides the solution that you need.

@maksim-petukhov
Copy link
Author

maksim-petukhov commented Aug 15, 2023

@postscript-dev As I said in the first post, this doesn't change anything for existing users of library. It is still good old C++98 when they do not use C++17 compiler mode. And there are no users who used C++17 standard with 0.27.x because it wasn't possible prior that PR. I tried to use Exiv2 0.28, but unfortunately in my project I can't open files with non-ASCII paths using it (see my comment). If I can't use 0.28 I have to use 0.27.x, but I use C++17 and 0.27.x is not C++17-compatible. So here we are.

@neheb
Copy link
Collaborator

neheb commented Aug 15, 2023

this is an API change.

I've seen libcxx fail with compilation as _LIBCPP_ENABLE_CXX17_REMOVED_AUTO_PTR is needed.

@maksim-petukhov
Copy link
Author

@neheb I'm sorry, but how does it change the API?

@neheb
Copy link
Collaborator

neheb commented Aug 15, 2023

You’re redefining AutoPtr as unique_ptr.

@maksim-petukhov
Copy link
Author

You’re redefining AutoPtr as unique_ptr.

Yes, but I'm doing it only for C++17 compiler mode so no users are affected by this API change. This library cannot be used with C++17 now, so there is no API at all.

@postscript-dev
Copy link
Collaborator

postscript-dev commented Aug 15, 2023

@maksim-petukhov
Whether this PR will be merged or not, I don't know. However, there are some things to bare in mind.

  1. As far as I understand it, the 0.27-maintenance branch is not under any active development and will only receive limited security fixes at best.

  2. As the project's resources are limited, our focus is on Exiv2 0.28.x. I think that we would usually encourage users to upgrade rather than work on an old version. Although upgrading is not an option for you.

  3. Exiv2 0.27.x has been removed from the project's nightly CI meaning we are not fuzzing, checking for any build issues or running the Exiv2 testing.

To summarise, although your PR might provide a solution it will cause several maintenance issues moving forward.

@kmilos
Copy link
Collaborator

kmilos commented Aug 16, 2023

GCC (as of v13) doesn't prevent you from compiling 0.27.x as c++17 AFAIK, and Clang compiles by simply defining _LIBCPP_ENABLE_CXX17_REMOVED_AUTO_PTR as mentioned. That leaves MSVC.

@neheb
Copy link
Collaborator

neheb commented Aug 16, 2023

google says for MSVC, _HAS_AUTO_PTR_ETC=1 needs to be defined. These two changes need to be done with CMakeLists.txt , not by changing headers and whatnot.

@kmilos
Copy link
Collaborator

kmilos commented Aug 17, 2023

These two changes need to be done with CMakeLists.txt

Not even, you can define on the command line when calling CMake (or from a build system of a project using exiv2) on a an as-needed basis, so no change to the 0.27 branch is required AFAICT...

@maksim-petukhov
Copy link
Author

@neheb @kmilos These defines are required not only to build exiv2. You have to define them for you project too because exiv2 uses auto_ptr in its interface. This can be OK for some "Hello World"-like program but not really appropriate for production code. There are internal guidelines, there are tendencies for conformance with C++ standard and these compiler defines are not compatible with these things. We can see how other people do this:

  • freeimage patched both in conan and vcpkg to use unique_ptr instead of auto_ptr
  • podofo patched in vcpkg to use unique_ptr instead of auto_ptr
  • libxmlpp patched in vcpkg to use unique_ptr instead of auto_ptr
  • coin-lemon patched in conan to remove register keyword
  • boost have similar BOOST_NO_AUTO_PTR to use unique_ptr instead of auto_ptr (see boost graph code for example)

Some of these libraries are not maintained AFAIK so you have to patch them. Having these necessary changes within the library itself is a more streamlined approach. This is the motivation behind my PR.

@codecov
Copy link

codecov bot commented Aug 20, 2023

Codecov Report

Merging #2743 (b4a6ca7) into 0.27-maintenance (3c648bc) will not change coverage.
The diff coverage is 73.41%.

@@                Coverage Diff                @@
##           0.27-maintenance    #2743   +/-   ##
=================================================
  Coverage             58.81%   58.81%           
=================================================
  Files                   149      149           
  Lines                 24476    24476           
  Branches              12669    12669           
=================================================
  Hits                  14395    14395           
  Misses                 7209     7209           
  Partials               2872     2872           
Files Changed Coverage Δ
include/exiv2/basicio.hpp 62.50% <ø> (ø)
include/exiv2/image.hpp 100.00% <ø> (ø)
include/exiv2/metadatum.hpp 40.00% <ø> (ø)
include/exiv2/value.hpp 75.00% <ø> (ø)
include/exiv2/xmp_exiv2.hpp 75.00% <ø> (ø)
src/actions.cpp 62.35% <0.00%> (ø)
src/actions.hpp 100.00% <ø> (ø)
src/bmpimage.cpp 19.51% <0.00%> (ø)
src/cr2image.cpp 8.98% <0.00%> (ø)
src/crwimage_int.hpp 87.17% <ø> (ø)
... and 28 more

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