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

Remove nonexisting "single_include" from INSTALL_INTERFACE #254

Conversation

ItsBasi
Copy link
Contributor

@ItsBasi ItsBasi commented Sep 27, 2021

CMake considers it an error that the target kompute::kompute references the "single_include" folder which is actually installed to "include".

CMakeLists.txt:-1: error: Imported target "kompute::kompute" includes non-existent path "D:/Downloads/kompute-0.8.0/build/install/single_include" in its INTERFACE_INCLUDE_DIRECTORIES. Possible reasons include: * The path was deleted, renamed, or moved to another location. * An install or uninstall procedure did not complete successfully. * The installation package was faulty and references files it does not provide.

CMake considers it an error that the target kompute::kompute references the "single_include" folder which is actually installed to "include".

CMakeLists.txt:-1: error: Imported target "kompute::kompute" includes non-existent path "D:/Downloads/kompute-0.8.0/build/install/single_include" in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include: * The path was deleted, renamed, or moved to another location. * An install or uninstall procedure did not complete successfully. * The installation package was faulty and references files it does not provide.

Signed-off-by: ItsBasi <5033630+ItsBasi@users.noreply.github.com>
@ItsBasi ItsBasi force-pushed the ItsBasi-patch-reference-to-non-existing-single_include branch from 8015c18 to 6b56fdd Compare September 27, 2021 20:24
@axsaucedo
Copy link
Member

Thank you for reporting this issue - however this doesn't seem correct as the single_include should also be included as part of the install interface. Are you saying that the single include is not being made available? If that is the case then the fix should be to make sure it's included

@ItsBasi
Copy link
Contributor Author

ItsBasi commented Sep 28, 2021

Right now cmake installs "${PROJECT_SOURCE_DIR}/single_include/" and "${PROJECT_SOURCE_DIR}/src/include/" into the identical destination which is "${CMAKE_INSTALL_PREFIX}/include/". But the package consumer looks for two folders ("${CMAKE_INSTALL_PREFIX}/include/" and "${CMAKE_INSTALL_PREFIX}/single_include/"), because the installer specified two folders for the install interface via "$<INSTALL_INTERFACE:include> $<INSTALL_INTERFACE:single_include>".

So the solution would either be...

1.) Remove the reference $<INSTALL_INTERFACE:single_include>
2.) Install "${PROJECT_SOURCE_DIR}/single_include/" to "${CMAKE_INSTALL_PREFIX}/single_include/"

I choose 1.) because that leaves the install layout unchanged.

Also i moved $<INSTALL_INTERFACE:include> to the install code section, which i think should be responsible for setting the right INSTALL_INTERFACE include.

@axsaucedo
Copy link
Member

Thank you for providing the full context @ItsBasi - looking at the suggestions I just need to clarify first the context, as option 1) seems to suggest that it would remove the single_include completely from the install package, which is not something we'd want given that we expect the single include to be part of the install. The second seems more relevant, as all the examples actually use the single include so we would need to make sure it's part of the install.

@ItsBasi
Copy link
Contributor Author

ItsBasi commented Sep 28, 2021

Option 1.) does not remove the header, as the single_include header is merged with the other one during installation. Therefore everything that links to the target "kompute" can find the single_include via "$<INSTALL_INTERFACE:include>".
I would stick to 1.) in order to avoid cluttering the install with two include folders.

Also i noticed that this has been discussed before in #212 and #213.

@axsaucedo
Copy link
Member

Oh right, thanks again for clarifying, it seems i was confused, as what you describe is indeed what we'd want - it would not make sense to have the folder explicitly defined as single_include in the resulting installation either way. Ok in that case this looks good, I'll test it locally and then should be good to merge. Thanks for the contribution

@axsaucedo axsaucedo merged commit 304ed84 into KomputeProject:master Sep 29, 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.

2 participants