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

Switch from CMAKE_x_DIR to PROJECT_x_DIR, thus isolating build artefacts #2711

Conversation

kamiccolo
Copy link
Contributor

@kamiccolo kamiccolo commented Jul 20, 2023

Briefly on CMake specifics:

${PROJECT_BINARY_DIR} - project specific binary directory
${PROJECT_SOURCE_DIR} - project specific root directory for the source (containing main CMakeLists.txt)

${CMAKE_CURRENT_x_DIR} - stands for source/binary directory for CMakeLists.txt currently being processed

${CMAKE_x_DIR} - "global" source directory, which may be from the parent project which includes Exiv2 by the means of FetchContent or add_subdirectory.

So, introduction of ${PROJECT_SOURCE_DIR} in the libraries code-base broke a possibility to include Exiv2 as a sub-project, for example briefly mentioned here: #2707

I took maybe a bit overly aggressive approach on moving everything to the scope of the project, so adding and building Exiv2 would no longer produce build artifacts in the following manner like it used to do in <0.28:

parent_project/build:
├── bin
│   ├── addmodel
│   ├── mmap-test
│   ├── conntest
│   ├── convert-test
...
├── app
│   ├── exiv2
├── exiv2lib_export.h
├── exv_conf.h
├── parent_project.h
├── parent_project.so
├── parent_project_executable
├── lib
│   ├── libexiv2.a
│   ├── libexiv2-xmp.a
│   ├── libgmock.a
│   ├── libgmock_main.a
│   ├── libgtest.a
│   └── libgtest_main.a

But the following:

parent_project/build:
├── parent_project.h
├── parent_project.so
├── parent_project_executable
├── exiv2
│   ├── bin
│   │   ├── addmodel
│   │   ├── mmap-test
│   │   ├── conntest
│   │   ├── convert-test
...
│   ├── app
│   │  ├── exiv2
│   ├── exiv2lib_export.h
│   ├── exv_conf.h
│   ├── lib
│   │   ├── libexiv2.a
│   │   ├── libexiv2-xmp.a
│   │   ├── libgmock.a
│   │   ├── libgmock_main.a
│   │   ├── libgtest.a
│   │   └── libgtest_main.a

Does that make sense?

EDIT: reverted most of the invasiveness, in such a way that x_BINARY_DIR and related behavior are left unchanged, but the handling of include dirs and x_SOURCE_DIR.

@ghost
Copy link

ghost commented Jul 20, 2023

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

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #2711 (7c20d06) into main (a2d6996) will not change coverage.
The diff coverage is n/a.

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

@kamiccolo
Copy link
Contributor Author

Uuuuu. I see, INSTALL target got messed up.

@kmilos
Copy link
Collaborator

kmilos commented Jul 20, 2023

I took maybe a bit overly aggressive approach on moving everything to the scope of the project, so adding and building Exiv2 would no longer produce build artifacts in the following manner like it used to do in <0.28

Yep, and the CI failures reflect this I guess. Isn't there a way to keep old behaviour when ${CMAKE_BINARY_DIR} equals ${PROJECT_BINARY_DIR}?

Edit: See e.g. how LibTIFF tried to do it. (I guess we also want to use ${CMAKE_CURRENT_LIST_DIR} when including other CMake files...)

@kamiccolo
Copy link
Contributor Author

kamiccolo commented Jul 20, 2023

I took maybe a bit overly aggressive approach on moving everything to the scope of the project, so adding and building Exiv2 would no longer produce build artifacts in the following manner like it used to do in <0.28

Yep, and the CI failures reflect this I guess. Isn't there a way to keep old behaviour when ${CMAKE_BINARY_DIR} equals ${PROJECT_BINARY_DIR}?

I believe so. Technically, first two of my commits do not even touch the BINARY_DIR's. But then some additional questions arises. For example, where to put exv_conf.h, exiv2lib_export.h? Kind of makes sense in both of the places. But what about doxygen docs, tests and gcov stuff? Which are only to be interfaced in the context of Exiv2.

EDIT: some of the includes goes up the currently processed directory. So, ${CMAKE_CURRENT_LIST_DIR} did not seem like an option (unless we'll do something like ${CMAKE_CURRENT_LIST_DIR}/../../ in OS agnostic way, which is no no no) :)

@kamiccolo
Copy link
Contributor Author

@kmilos I believe I've managed to reduce the surface and impact only on to the include paths and CMAKE_MODULES being served. Still kind of bamboozled why the write_basic_package_version_file(exiv2ConfigVersion.cmake COMPATIBILITY ExactVersion) have failed the first time o_0 Or rather used different directory for the output INSTALL target could not pick-up.

@kmilos
Copy link
Collaborator

kmilos commented Jul 21, 2023

So, does this resolve #2707 fully now?

@kamiccolo
Copy link
Contributor Author

So, does this resolve #2707 fully now?

Thanks a lot, it does indeed!

With this PR the scenario when the libexiv2 is included using CMake's FetchContent and/or add_subdirectory() is viable again!

@neheb neheb merged commit 5631e84 into Exiv2:main Jul 21, 2023
105 checks passed
@neheb
Copy link
Collaborator

neheb commented Jul 22, 2023

@Mergifyio backport 0.28.x

@mergify
Copy link
Contributor

mergify bot commented Jul 22, 2023

backport 0.28.x

✅ Backports have been created

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.

3 participants