[suitesparse] Fixing SuiteSparse_INCLUDE_DIRS is not usable from the port.

[seanyen]

The suitesparse exports a CMake variable SuiteSparse_INCLUDE_DIRS, which is pointing to a non-existing location from this Vcpkg port. However this doesn't repro if I skip using Vcpkg and go directly build it from the upstream. There are some delta between Vcpkg port and the upstream.

It turned out, in this suitesparse Vcpkg port, it moves its CMake config files into share/suitesparse/ from lib/cmake/suitesparse-xxx by vcpkg_fixup_cmake_targets. However, the config file computes the installed root by assuming it is under lib/cmake/suitesparse-xxx, and this produces an unusable CMake config file in Vcpkg port.

This pull request is to fix up the computation logic to accommodate the new location (share/suitesparse/) by Vcpkg port.

[seanyen]

This is ready for review and merge.

[JackBoosY]

Hi @seanyen, in vcpkg, the cmake files generated by ports should be installed in VCPKG_PATH/installed/${TRIPLET}/share/${PORT} so that users can use the vcpkg toolchain to find them, which is by design.
And you can see vcpkg_fixup_cmake_targets is doing this.

And by using vcpkg toolchain, I can use the port correctly.

So I don't think we should fix it.

[seanyen]

@JackBoosY This is really interesting. What's your test case? I am using the vcpkg toolchain too, with Visual Studio 2019.

I can share my setup for repro. Here is the CMakeList.txt:

# CMakeList.txt : CMake project for CMakeProject2, include source and define
# project specific logic here.
#
cmake_minimum_required (VERSION 3.8)

project ("CMakeProject2")

find_package(suitesparse REQUIRED)

include_directories(${SuiteSparse_INCLUDE_DIRS})

add_executable (CMakeProject2 "CMakeProject2.cpp" "CMakeProject2.h")
target_link_libraries(CMakeProject2 ${SuiteSparse_LIBRARIES})

And here is the cpp file:

// CMakeProject2.cpp : Defines the entry point for the application.
//
#include <iostream>
#include "suitesparse/cs.h"

using namespace std;

int main()
{
	cs_spfree(nullptr);
	cout << "Hello CMake." << endl;
	return 0;
}

Without the fix, the ${SuiteSparse_INCLUDE_DIRS} will be pointing to VCPKG_PATH/installed/include opposing to VCPKG_PATH/installed/${TRIPLET}/include. Subsequently, it will be a compile error because it cannot find suitesparse/cs.h.

Let me know if you see otherwise.

[JackBoosY]

I think you need to use find_package(suitesparse CONFIG REQUIRED) instead.
If you do not use the keyword CONFIG, it will use Findsuitespares.cmake.

[JackBoosY]

My test case is:

cmake_minimum_required (VERSION 3.8)

# Add source to this project's executable.
add_executable (CMakeProject2 "CMakeProject2.cpp" "CMakeProject2.h")

# TODO: Add tests and install targets if needed.
find_package(suitesparse CONFIG REQUIRED)

target_link_libraries(CMakeProject2 PRIVATE SuiteSparse::amd SuiteSparse::btf SuiteSparse::klu SuiteSparse::ldl)

Configure log:

1> Command line: C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2017\ENTERPRISE\COMMON7\IDE\COMMONEXTENSIONS\MICROSOFT\CMAKE\CMake\bin\cmake.exe  -G "Visual Studio 15 2017 Win64" -DCMAKE_INSTALL_PREFIX:PATH="C:\Users\\CMakeBuilds\c23cdb84-3680-0e3b-a405-00277fbe5082\install\x86-Debug"  -DCMAKE_TOOLCHAIN_FILE="F:/vcpkg/scripts/buildsystems/vcpkg.cmake"  -DCMAKE_CXX_COMPILER="C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/HostX86/x64/cl.exe"  -DCMAKE_C_COMPILER="C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.16.27023/bin/HostX86/x64/cl.exe"  -DCMAKE_CONFIGURATION_TYPES="Debug" "C:\Users\\source\repos\CMakeProject2"
1> Working directory: C:\Users\\CMakeBuilds\c23cdb84-3680-0e3b-a405-00277fbe5082\build\x86-Debug
1> -- Found CLAPACK: optimized;F:/vcpkg/installed/x64-windows/lib/lapack.lib;optimized;F:/vcpkg/installed/x64-windows/lib/libf2c.lib;debug;F:/vcpkg/installed/x64-windows/debug/lib/lapackd.lib;debug;F:/vcpkg/installed/x64-windows/debug/lib/libf2cd.lib;F:/vcpkg/installed/x64-windows/debug/lib/openblas.lib;F:/vcpkg/installed/x64-windows/debug/lib/openblas.lib;F:/vcpkg/installed/x64-windows/debug/lib/openblas.lib  
1> -- Found LAPACK: optimized;F:/vcpkg/installed/x64-windows/lib/lapack.lib;optimized;F:/vcpkg/installed/x64-windows/lib/libf2c.lib;debug;F:/vcpkg/installed/x64-windows/debug/lib/lapackd.lib;debug;F:/vcpkg/installed/x64-windows/debug/lib/libf2cd.lib;F:/vcpkg/installed/x64-windows/debug/lib/openblas.lib;F:/vcpkg/installed/x64-windows/debug/lib/openblas.lib;F:/vcpkg/installed/x64-windows/debug/lib/openblas.lib  
1> -- Configuring done
1> -- Generating done
1> -- Build files have been written to: C:/Users//CMakeBuilds/c23cdb84-3680-0e3b-a405-00277fbe5082/build/x86-Debug
1> Starting CMake target info extraction ...
1> CMake server connection made.
1> Extracted includes paths.
1> Extracted CMake variables.
1> Extracted source files and headers.
1> Extracted global settings.
1> Extracted code model.
1> Extracted CTest info.
1> Collating data ...
1> Target info extraction done.

[seanyen]

@JackBoosY Thanks. You are talking about imported target but here what I am talking is the xxx_INCLUDE_DIRS and xxx__LIBRARIES convention. I know the imported target is the more modern CMake way, but many downstream projects are not yet fully adopting that.

And I would say Vcpkg port should try best to keep the behavior supported by the upstream, instead of breaking it.

[JackBoosY]

@seanyen In general, the use of macros(such as xxx_INCLUDE_DIRS and xxx_LIBRARIES) to use the port is through Find${PORT}.cmake or manually implement find_library to achieve.
And the suitesparse officially provides the imported target way.
After checking the cmake documentation, the official cmake did not provide findsuitesparse.cmake.

So I don’t know where SuiteSparse_INCLUDE_DIRS and SuiteSparse_LIBRARIES are provided.

[seanyen]

So I don’t know where SuiteSparse_INCLUDE_DIRS and SuiteSparse_LIBRARIES are provided.

This. https:/jlblancoc/suitesparse-metis-for-windows/blob/master/cmake/SuiteSparse-config-install.cmake.in#L20-L38

And that's what I patched.

[JackBoosY]

@seanyen If the suitesparese official does not accept this modification, I think it is difficult for us to accept this PR.

[seanyen]

@JackBoosY

Thanks. I think the main point is that some CMake variable (SuiteSparse_INCLUDE_DIRS) exported by suitesparse-metis-for-windows is broken from Vcpkg port.

The issue doesn't exist if I skip Vcpkg and go directly with the upstream (suitesparse-metis-for-windows), so I am not really sure if creating any pull request would make sense to the upstream.

If you agree with me that SuiteSparse_INCLUDE_DIRS is not usable from this Vcpkg port, then I am open to any feedback how we fix this problem together.

And just for your information, we are the Microsoft Edge Robotics team (https://aka.ms/ros) and we are helping bootstrap the dependencies for ROS community on Windows (suitesparse is one of that and that's why we found this issue). And I hope we can figure out a way to make Windows developers life easier. :)

[JackBoosY]

@vicroms Could you please review this PR?

Thanks.

[seanyen]

@vicroms friendly ping. Any feedback?

[vicroms]

Thanks for the PR!