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

fix_1516_include_path #1565

Merged
merged 1 commit into from
Aug 10, 2021
Merged

fix_1516_include_path #1565

merged 1 commit into from
Aug 10, 2021

Conversation

clanmills
Copy link
Collaborator

See #1516.

@clanmills clanmills added this to the v1.00 milestone Apr 18, 2021
@clanmills clanmills requested a review from piponazo April 18, 2021 10:37
@clanmills clanmills self-assigned this Apr 18, 2021
Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment indicating why I think we do not need this change. But if you prove me wrong (give me some steps so that I can reproduce the problem) I will double-check.

@@ -79,6 +79,8 @@ if( EXPAT_FOUND )
exiv2-xmp
${EXPAT_LIBRARIES}
)
target_include_directories(geotag PRIVATE ${CMAKE_BINARY_DIR}) # exv_conf.h
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Robin, I think these two lines are not needed.

We are including the ${CMAKE_BINARY_DIR} globally for all the project targets at CMakeLists.txt:78.

The ${CMAKE_SOURCE_DIR}/include directory is also propagated when we add the dependency of each sample target to the library exiv2:

  • On samples/CMakeLists.txt:128 we are adding the dependency to the exiv2 target.
  • On src/CMakeLists.txt:150-154 we indicated the directoryes which are propagated when some target depends on exiv2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see, I'll try to reproduce it on my side ... It should see first the headers specified in the project rather than something you have in /usr/local/inclde

@clanmills clanmills removed their assignment Apr 19, 2021
@kevinbackhouse
Copy link
Collaborator

I am not able to reproduce this problem on Ubuntu 21.04. Here's what I tried:

  • sudo apt-get install libexiv2-dev
  • Add #error to /usr/include/exiv2/exiv2.hpp and /usr/include/exiv2/exv_conf.h so that the build will fail if either of those header files is used.
  • make clean; make

Or is this only a macOS problem?

@codecov
Copy link

codecov bot commented Aug 7, 2021

Codecov Report

Merging #1565 (c82c4d0) into main (a1b65a7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1565   +/-   ##
=======================================
  Coverage   60.78%   60.78%           
=======================================
  Files          96       96           
  Lines       18881    18881           
  Branches     9494     9494           
=======================================
  Hits        11476    11476           
  Misses       5115     5115           
  Partials     2290     2290           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1b65a7...c82c4d0. Read the comment docs.

@kevinbackhouse
Copy link
Collaborator

@clanmills: All the checks are passing, so I think this is fine to merge. Should it be back-ported to 0.27-maintenance?

@kevinbackhouse kevinbackhouse merged commit ced25a5 into main Aug 10, 2021
@mergify mergify bot deleted the fix_1516_include_path branch August 10, 2021 13:55
@kevinbackhouse
Copy link
Collaborator

@Mergifyio backport 0.27-maintenance

@mergify
Copy link
Contributor

mergify bot commented Aug 10, 2021

Command backport 0.27-maintenance: success

Backports have been created

clanmills added a commit that referenced this pull request Aug 10, 2021
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