cmake: Fix paths with absolute GNUInstallDirs#1263
Conversation
src/exiv2.cpp
Outdated
| #ifdef EXV_ENABLE_NLS | ||
| setlocale(LC_ALL, ""); | ||
| const std::string localeDir = Exiv2::getProcessPath() + EXV_LOCALEDIR; | ||
| const std::string localeDir = EXV_LOCALEDIR[0] == '/' ? EXV_LOCALEDIR : Exiv2::getProcessPath() + "/" + EXV_LOCALEDIR; |
There was a problem hiding this comment.
It would be nicer to be able to use std::filesystem::path(Exiv2::getProcessPath()) / EXV_LOCALEDIR but that requires C++17:
There was a problem hiding this comment.
@tbeu is correct. Currently Exiv2 v0.27-maintenance is written to the C++98 standard. When it compiles with C++11 or C++14, you should set the flags: -DCMAKE_CXX_STANDARD=11 -DCMAKE_CXX_FLAGS=-Wno-deprecated. 0.27-maintenance cannot be built with C++17 and later because it uses auto_ptr.
The 'master' branch builds with C++11. When we decide to "modernise" the C++ code, KDE requested that we avoid dependency on C++14 and later.
Exiv2 has always avoided Boost. We want to be able to build Exiv2 with the platform standard C++ compiler and minimal dependencies. To support XMP, we require Expat. To support PNG, we require zlib. Both XMP and PNG are optional. All dependencies (localisation, charset conversions, gtest and curl) are optional.
There was a problem hiding this comment.
Yeah, I suspected something like that. Fortunately, with conditional expressions it is not that much longer.
|
I'm not an expert on CMake (or anything else for that matter), so I've asked @piponazo to review. I know that something was done concerning GNUInstallDirs in the past. After this PR has been reviewed and merged, can you submit an equivalent PR to branch 0.27-maintenance which is used to maintain the 0.27 "dots". Until 'master' ships as Exiv2 v0.28, the current "shipping branch" is 0.27-maintenance. |
|
The CI failures look unrelated. |
Codecov Report
@@ Coverage Diff @@
## master #1263 +/- ##
=======================================
Coverage 71.91% 71.91%
=======================================
Files 152 152
Lines 17674 17674
=======================================
Hits 12710 12710
Misses 4964 4964
Continue to review full report at Codecov.
|
|
Dan set up the the CI and I don't understand it. It fails almost every build for reasons that I usually cannot understand. I have a build server at home on my MacMini and I build on the 8 supported platforms: Ubuntu, macOS, cygwin/64, MinGW/msys2, msvc2019, solaris, freebsd and netbsd. The github/CI builds about 50 platforms (suse, fedora and loads of others). It doesn't build some supported platforms (cygwin/64 and MinGW/msys2). Even when the CI is successful, CodeCov is happy to produce other inexplicable build failures. |
|
Do you have plans to release 0.27.4? I could open a PR targetting the maintenance branch to at least fix it there before the next release and leave this until some maintainer of master branch appears. |
|
Please submit PRs to branch 0.27-maintenance which is scheduled for release as 0.27.4 in Spring 2021. It's hard to predict anything, especially the future! Sadly, I think branch 'master' has died as almost nothing has been contributed in 2020. I returned to the project in March 2020 to finish 0.27.3 on branch 0.27-maintenance. I hoped to motivate the others to use their covid/lockdown time to work on branch 'master'. |
|
Opened #1275 |
It is not generally true that `CMAKE_INSTALL_<dir>` variables are relative paths: https://github.com/jtojnar/cmake-snips#concatenating-paths-when-building-pkg-config-files https://github.com/jtojnar/cmake-snips#assuming-cmake_install_dir-is-relative-path Let's join them properly as paths, not strings. Signed-off-by: Jan Tojnar <jtojnar@gmail.com> (cherry-picked from 48f2c9d)
piponazo
left a comment
piponazo
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I never thought about the case of assuming relative paths you explain in one of the links provided ... but it sounds reasonable!
If this has been already merged in 0.27-maintenance without creating troubles, I think it would be also nice to have it in master 😉
|
Great to hear your voice, @piponazo Hope everything's great with you and yours. I merged #1275 into 0.27-maintenance yesterday. So, you can merge this into 'master'. I have a super young Chinese guy (23 years old) working with me on the test suite (getting rid of bash). Very pleased with his work. I'm going to get him to do some testing on our Unicode support. |
4 participants
It is not generally true that
CMAKE_INSTALL_<dir>variables are relative paths:https://github.com/jtojnar/cmake-snips#concatenating-paths-when-building-pkg-config-files
https://github.com/jtojnar/cmake-snips#assuming-cmake_install_dir-is-relative-path
Let's join them properly as paths, not strings.