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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add include directory based on CMAKE variables to assimp #35

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@alicemargatroid
Contributor

alicemargatroid commented Sep 3, 2017

Looks like someone missed the ASSIMP_INCLUDE_DIR variable 馃憥

@coveralls

This comment has been minimized.

coveralls commented Sep 3, 2017

Coverage Status

Coverage remained the same at 85.285% when pulling a7c79a9 on alicemargatroid:master into a1a72f1 on mosra:master.

@Squareys

This comment has been minimized.

Contributor

Squareys commented Sep 4, 2017

Assimp related code is probably me! 馃槄

The problem is somewhere else, though:

 target_link_libraries(AssimpImporter Magnum::Magnum Assimp::Assimp)

uses an imported target Assimp::Assimp which is defined in FindAssimp.cmake and usually automatically sets the include directory (defined here)! It is probably not found there (In which cmake configure shouldn't be able to succeed, though, thanks to this line, so I am rather confused.

@alicemargatroid Which system are you on? I remember this being enabled on at least one CI, so this must be a platform specific problem.

@mosra

This comment has been minimized.

Owner

mosra commented Sep 4, 2017

@alicemargatroid oh! terribly sorry about this. Thanks for the patch!

@Squareys the line you mean affects AssimpImporter, but not AssimpImporterObjects .. in our case (and also in case of the CI, unfortunately) AssImp was installed in the default locations, making the includes "just work". Putting its headers into some non-implicit location exposes this problem.

@alicemargatroid

This comment has been minimized.

Contributor

alicemargatroid commented Sep 4, 2017

@mosra No problem, very easy oversight to make; would be nice to see this PR merged though, so I can stop using my own fork!

@Squareys This is not platform specific; I compile everything myself (I don't use the package manager or whatever to install my dependencies), so I need Magnum to know where I've assigned ASSIMP_INCLUDE_DIR, otherwise it won't pick up my includes.

Spent a day and a half scratching my head over this before just directly diff'ing the CMakeLists.txt of JpegImporter vs AssimpImporter, and it turned out to be this tiny innocuous line :)

Can confirm this fixes builds for me on Ubuntu, MingW, and OSX.

@mosra

This comment has been minimized.

Owner

mosra commented Sep 4, 2017

Merged as 530c75c -- I replaced ${ASSIMP_INCLUDE_DIR} with $<TARGET_PROPERTY:Assimp::Assimp,INTERFACE_INCLUDE_DIRECTORIES> to avoid using internal variables of the Find module.

Hope I didn't break it for you again with that change :D

@mosra mosra closed this Sep 4, 2017

@alicemargatroid

This comment has been minimized.

Contributor

alicemargatroid commented Sep 4, 2017

@mosra

This comment has been minimized.

Owner

mosra commented Sep 4, 2017

Because FindJPEG.cmake doesn't have imported targets yet ...

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment