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

CMake Error: Imported target "kompute::kompute" includes non-existent path "/usr/local/single_include" #212

Closed
unexploredtest opened this issue May 3, 2021 · 13 comments

Comments

@unexploredtest
Copy link
Contributor

When Vulkan Kompute is installed globally, it'll try to find /usr/local/single_include but it doesn't exist, the cause of problem is from:

target_include_directories(
    kompute PUBLIC
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
    $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/single_include>
    $<INSTALL_INTERFACE:include>
    $<INSTALL_INTERFACE:single_include>
)

In src/CMakeLists.txt.

@unexploredtest
Copy link
Contributor Author

unexploredtest commented May 3, 2021

One solution I can think of is if one has enabled KOMPUTE_OPT_INSTALL, then I don't think they'll need to include anything from single_include(correct?), as it gets installed in include/kompute/Kompute.hpp. So we can include single_include just when KOMPUTE_OPT_INSTALL i set to 0:

target_include_directories(
    kompute PUBLIC
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
    $<INSTALL_INTERFACE:include>
)

# Including Kompute.hpp from single_include if Vulkan Kompute is not installed
# If Vulkan Kompute is installed and we include these directories, cmake will try 
# to find a path under a non-existance directory (because single_include doesn't get added)
if(NOT KOMPUTE_OPT_INSTALL)
    target_include_directories(
        kompute PUBLIC
        $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/single_include>
        $<INSTALL_INTERFACE:single_include>
    )
endif()

@axsaucedo
Copy link
Member

Good shout @unexploredtest, good to have the discussion on the chat docuemnted as an issue. L

ooking at this proposed solution generally it does make sense, however in the context of the examples (as per the example folder), users may want to include the kompute dependency via cmake import, and may want to use it in their projects through the kompute.hpp without enabling install.

Also, just to confirm, the expected way of using the single include is via #include "kompute/Kompute.hpp", so there is no need to have a single_include top level folder, As long as there is a path containing Kompute.hpp under kompute folder it should work. Is there a reason why you would expect the single_include folder?

@unexploredtest
Copy link
Contributor Author

Looking at this proposed solution generally it does make sense, however in the context of the examples (as per the example folder), users may want to include the kompute dependency via cmake import, and may want to use it in their projects through the kompute.hpp without enabling install.

Yeah I see, I just need a bit of clarification. For example, in the example 'array_multiplication', you mean first building Kompute without install and then using find_package, or just including Kompute as a subdirectory?

Also, just to confirm, the expected way of using the single include is via #include "kompute/Kompute.hpp", so there is no need to have a single_include top level folder, As long as there is a path containing Kompute.hpp under kompute folder it should work. Is there a reason why you would expect the single_include folder?

With or without installation?

@axsaucedo
Copy link
Member

Yeah I see, I just need a bit of clarification. For example, in the example 'array_multiplication', you mean first building Kompute without install and then using find_package, or just including Kompute as a subdirectory?

That is correct, you can see that in the cmakelists it only imports the project (as uses by default not KOMPUTE_ARR_OPT_INSTALLED_KOMPUTE) - it then allows for the kompute/Kompute.hpp file to be used

With or without installation?

In both cases you would reference the file as kompute/Kompute.hpp, the single_include folder is not really part of the expected path.

@unexploredtest
Copy link
Contributor Author

That is correct, you can see that in the cmakelists it only imports the project (as uses by default not KOMPUTE_ARR_OPT_INSTALLED_KOMPUTE) - it then allows for the kompute/Kompute.hpp file to be used

I see,

In both cases you would reference the file as kompute/Kompute.hpp, the single_include folder is not really part of the expected path.

Makes sense, but I'm a bit confused, why $<INSTALL_INTERFACE:single_include> was added in the first place? I see that in the previous releases (0.7.0 and 0.6.0)

target_include_directories(
    kompute PUBLIC
    ${CMAKE_CURRENT_SOURCE_DIR}/include
    ${PROJECT_SOURCE_DIR}/single_include
)

was used, was there an issue with this approach?

@axsaucedo
Copy link
Member

was used, was there an issue with this approach?

Good point, I think there were issues I found when the explicit interfaces were not being used, particularly in the context of the examples like the android one. However it does seem like you have been able to take it to the test, and if we can go back to this simpler appraoch it would be much better to be honest.

@unexploredtest
Copy link
Contributor Author

unexploredtest commented May 3, 2021

Good point, I think there were issues I found when the explicit interfaces were not being used, particularly in the context of the examples like the android one. However it does seem like you have been able to take it to the test, and if we can go back to this simpler appraoch it would be much better to be honest.

I see, makes sense. In my case it appears that logistic_regression and array_multiplication work fine with the first approach. I wish I could test the android example but unfortunately I don't have a Vulkan-enabled phone... What kind of issues were raised? If it's only with the Android example, maybe we can include these explicit interfaces only if KOMPUTE_OPT_ANDOID_BUILD is enabled.

@axsaucedo
Copy link
Member

maybe we can include these explicit interfaces only if KOMPUTE_OPT_ANDOID_BUILD is enabled.

Good suggestion, I think for now it woudl make sense to go back to the simpler version. If you open a PR we can merge going back to that, and then as we approach the next release I can make sure all examples run with the relevant conditional statements that are specific to those.

@unexploredtest
Copy link
Contributor Author

If you open a PR we can merge going back to that

Sure! With the new suggestion?

@axsaucedo
Copy link
Member

Yes exactly - with the one you suggested above, namely:

target_include_directories(
    kompute PUBLIC
    ${CMAKE_CURRENT_SOURCE_DIR}/include
    ${PROJECT_SOURCE_DIR}/single_include
)

Assuming that it would ensure both are included in the build and install interface

@unexploredtest
Copy link
Contributor Author

Fixed with #213

@unexploredtest
Copy link
Contributor Author

unexploredtest commented May 15, 2021

So, how about

target_include_directories(
    kompute PUBLIC
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
    $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/single_include>
    $<INSTALL_INTERFACE:include>
)

if(NOT KOMPUTE_OPT_INSTALL)
    target_include_directories(
        kompute PUBLIC
    $<INSTALL_INTERFACE:single_include>
    )
endif()

Because I when we run examples, KOMPUTE_OPT_INSTALL is ultimately off and therefor, I think it'll be safe and also solves the issue

@axsaucedo
Copy link
Member

This should now be fixed , and with glslang removed should be more robust

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

No branches or pull requests

2 participants