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

[cdt] Add new port #29324

Merged
merged 4 commits into from
Mar 28, 2023
Merged

[cdt] Add new port #29324

merged 4 commits into from
Mar 28, 2023

Conversation

ex-purple
Copy link
Contributor

@ex-purple ex-purple commented Jan 31, 2023

Fixes #28238.

  • Changes comply with the maintainer guide
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See docs/examples/adding-an-explicit-usage.md for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

github-actions[bot]
github-actions bot previously approved these changes Jan 31, 2023
@BillyONeal
Copy link
Member

I manually checked that the SHA in here is not affected by #29288

@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Feb 1, 2023
ports/cdt/vcpkg.json Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Feb 1, 2023
JonLiu1993
JonLiu1993 previously approved these changes Feb 2, 2023
@JonLiu1993
Copy link
Member

JonLiu1993 commented Feb 2, 2023

All features are tested successfully in the following triplet:
- x86-windows
- x64-windows
- x64-windows-static

Tested usage successfully by the cdt:x64-windows triplet

cdt provides CMake targets:

    # this is heuristically generated, and may not be correct
    find_package(CDT CONFIG REQUIRED)
    target_link_libraries(main PRIVATE CDT::CDT)

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Feb 2, 2023
FEATURES
"64-bit-index-type" CDT_USE_64_BIT_INDEX_TYPE
"as-compiled-library" CDT_USE_AS_COMPILED_LIBRARY
"boost" CDT_USE_BOOST
Copy link
Member

Choose a reason for hiding this comment

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

This looks like an alternative but it doesn't appear to leak into any ABI sensitive bits, so it's probably OK. However, the value does need to be "baked" into the resulting headers. See https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#a-feature-may-replace-polyfills-with-aliases-provided-that-replacement-is-baked-into-the-installed-tree

I think you can do this with something like:

if(CDT_USE_BOOST)
set(CDT_USE_BOOST_STR "#if 1")
else()
set(CDT_USE_BOOST_STR "#if 0")
endif()

foreach(FILE CDTUtils.h Triangulation.hpp)
  vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/${FILE}" "#ifdef CDT_USE_BOOST" "${CDT_USE_BOOST_STR}")
endforeach()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

ports/cdt/vcpkg.json Outdated Show resolved Hide resolved
@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Feb 3, 2023
@BillyONeal
Copy link
Member

All features are tested successfully in the following triplet

How did it find boost for you? 👀

@JonLiu1993
Copy link
Member

JonLiu1993 commented Feb 3, 2023

All features are tested successfully in the following triplet

How did it find boost for you? 👀

Sorry, I tested the feature and usage of several PRs locally at the same time. After testing the usage of this port, I thought I had already tested its feature, so I wrote it like that and this caused this error. I have always been in the local The test result will be written after the test is completed. If the test fails, I will provide the user with an error log.

usage test:

1> CMake generation started for configuration: 'x64-Debug'.
1> The toolchain file has changed (CMAKE_TOOLCHAIN_FILE).
1> Command line: "C:\WINDOWS\system32\cmd.exe" /c "%SYSTEMROOT%\System32\chcp.com 65001 >NUL && "C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2019\ENTERPRISE\COMMON7\IDE\COMMONEXTENSIONS\MICROSOFT\CMAKE\CMake\bin\cmake.exe"  -G "Visual Studio 16 2019" -A x64  -DCMAKE_CONFIGURATION_TYPES:STRING="Debug" -DCMAKE_INSTALL_PREFIX:PATH="C:\Users\test\source\repos\CMakeProject42\out\install\x64-Debug" -DCMAKE_TOOLCHAIN_FILE=F:/Feature-test/vcpkg/scripts/buildsystems/vcpkg.cmake "C:\Users\test\source\repos\CMakeProject42" 2>&1"
1> Working directory: C:\Users\test\source\repos\CMakeProject42\out\build\x64-Debug
1> [CMake] -- Selecting Windows SDK version 10.0.19041.0 to target Windows 10.0.19045.
1> [CMake] -- The C compiler identification is MSVC 19.29.30146.0
1> [CMake] -- The CXX compiler identification is MSVC 19.29.30146.0
1> [CMake] -- Detecting C compiler ABI info
1> [CMake] -- Detecting C compiler ABI info - done
1> [CMake] -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe - skipped
1> [CMake] -- Detecting C compile features
1> [CMake] -- Detecting C compile features - done
1> [CMake] -- Detecting CXX compiler ABI info
1> [CMake] -- Detecting CXX compiler ABI info - done
1> [CMake] -- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe - skipped
1> [CMake] -- Detecting CXX compile features
1> [CMake] -- Detecting CXX compile features - done
1> [CMake] -- Configuring done
1> [CMake] -- Generating done
1> [CMake] -- Build files have been written to: C:/Users/test/source/repos/CMakeProject42/out/build/x64-Debug
1> Extracted CMake variables.
1> Extracted source files and headers.
1> Extracted code model.
1> Extracted toolchain configurations.
1> Extracted includes paths.
1> CMake generation finished.

Build :

Build started...
1>------ Build started: Project: ZERO_CHECK, Configuration: Debug x64 ------
1>Checking Build System
2>------ Build started: Project: main, Configuration: Debug x64 ------
2>Building Custom Rule C:/Users/test/source/repos/CMakeProject42/CMakeProject42/CMakeLists.txt
2>CMakeProject42.cpp
2>main.vcxproj -> C:\Users\test\source\repos\CMakeProject42\out\build\x64-Debug\CMakeProject42\Debug\main.exe
3>------ Skipped Build: Project: ALL_BUILD, Configuration: Debug x64 ------
3>Project not selected to build for this solution configuration 
========== Build: 2 succeeded, 0 failed, 0 up-to-date, 1 skipped ==========

@BillyONeal
Copy link
Member

Yeah it looks like the boost feature wasn't turned on in that test. Oh well 🤷

github-actions[bot]
github-actions bot previously approved these changes Feb 15, 2023
JonLiu1993
JonLiu1993 previously approved these changes Feb 17, 2023
@JonLiu1993
Copy link
Member

@ex-purple, when I test the feature by command ./vcpkg install cdt[64-bit-index-type,as-compiled-library,boost]:x64-windows I get this error, please take a look:

CMake Error at scripts/cmake/vcpkg_execute_build_process.cmake:134 (message):
    Command failed: F:/Feature-test/cdt/vcpkg/downloads/tools/cmake-3.25.1-windows/cmake-3.25.1-windows-i386/bin/cmake.exe --build . --config Debug --target install -- -v -j9
    Working Directory: F:/Feature-test/cdt/vcpkg/buildtrees/cdt/x64-windows-dbg
    See logs for more information:
      F:\Feature-test\cdt\vcpkg\buildtrees\cdt\install-x64-windows-dbg-out.log

Call Stack (most recent call first):
  installed/x64-windows/share/vcpkg-cmake/vcpkg_cmake_build.cmake:74 (vcpkg_execute_build_process)
  installed/x64-windows/share/vcpkg-cmake/vcpkg_cmake_install.cmake:16 (vcpkg_cmake_build)
  ports/cdt/portfile.cmake:26 (vcpkg_cmake_install)
  scripts/ports.cmake:147 (include)

F:\Feature-test\cdt\vcpkg\buildtrees\cdt\install-x64-windows-dbg-out.log

[1/3] C:\PROGRA~1\MIB055~1\2022\COMMUN~1\VC\Tools\MSVC\1434~1.319\bin\Hostx64\x86\cl.exe   /TP -DCDT_EXPORTS -DCDT_USE_64_BIT_INDEX_TYPE -DCDT_USE_AS_COMPILED_LIBRARY -DCDT_USE_BOOST -IF:\Feature-test\cdt\vcpkg\buildtrees\cdt\x86-windows-dbg -IF:\Feature-test\cdt\vcpkg\buildtrees\cdt\src\1.2.0-dbb5670f32.clean\CDT\include -IF:\Feature-test\cdt\vcpkg\buildtrees\cdt\src\1.2.0-dbb5670f32.clean\CDT\extras /nologo /DWIN32 /D_WINDOWS /W3 /utf-8 /GR /EHsc /MP  /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1 /showIncludes /FoCMakeFiles\CDT.dir\src\CDT.cpp.obj /FdCMakeFiles\CDT.dir\ /FS -c F:\Feature-test\cdt\vcpkg\buildtrees\cdt\src\1.2.0-dbb5670f32.clean\CDT\src\CDT.cpp
FAILED: CMakeFiles/CDT.dir/src/CDT.cpp.obj 
C:\PROGRA~1\MIB055~1\2022\COMMUN~1\VC\Tools\MSVC\1434~1.319\bin\Hostx64\x86\cl.exe   /TP -DCDT_EXPORTS -DCDT_USE_64_BIT_INDEX_TYPE -DCDT_USE_AS_COMPILED_LIBRARY -DCDT_USE_BOOST -IF:\Feature-test\cdt\vcpkg\buildtrees\cdt\x86-windows-dbg -IF:\Feature-test\cdt\vcpkg\buildtrees\cdt\src\1.2.0-dbb5670f32.clean\CDT\include -IF:\Feature-test\cdt\vcpkg\buildtrees\cdt\src\1.2.0-dbb5670f32.clean\CDT\extras /nologo /DWIN32 /D_WINDOWS /W3 /utf-8 /GR /EHsc /MP  /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1 /showIncludes /FoCMakeFiles\CDT.dir\src\CDT.cpp.obj /FdCMakeFiles\CDT.dir\ /FS -c F:\Feature-test\cdt\vcpkg\buildtrees\cdt\src\1.2.0-dbb5670f32.clean\CDT\src\CDT.cpp
F:\Feature-test\cdt\vcpkg\buildtrees\cdt\src\1.2.0-dbb5670f32.clean\CDT\include\CDTUtils.h(49): fatal error C1083: Cannot open include file: 'boost/container/flat_set.hpp': No such file or directory
ninja: build stopped: subcommand failed.

@ex-purple ex-purple dismissed stale reviews from JonLiu1993 and github-actions[bot] via 85eee7c March 2, 2023 18:04
@ex-purple
Copy link
Contributor Author

@JonLiu1993 I added a patch
Please try again

@ex-purple ex-purple requested a review from JonLiu1993 March 2, 2023 18:05
@JonLiu1993
Copy link
Member

[64-bit-index-type,as-compiled-library,boost] feature tested successfully in the following triplet:

  • x86-windows
  • x64-windows
  • x64-windows-static

but when I tested usage by cdt:x64-windows failed:

CMakeLists:

cmake_minimum_required (VERSION 3.8)

project(test)

find_package(CDT CONFIG REQUIRED)

add_executable (main "CMakeProject42.cpp")

target_link_libraries(main PRIVATE CDT::CDT)

CMakeProject42.cpp:

#include <iostream>
#include "CDT.h"

using namespace std;

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

config:

1> CMake generation started for configuration: 'x64-Debug'.
1> Command line: "C:\WINDOWS\system32\cmd.exe" /c "%SYSTEMROOT%\System32\chcp.com 65001 >NUL && "C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2019\ENTERPRISE\COMMON7\IDE\COMMONEXTENSIONS\MICROSOFT\CMAKE\CMake\bin\cmake.exe"  -G "Visual Studio 16 2019" -A x64  -DCMAKE_CONFIGURATION_TYPES:STRING="Debug" -DCMAKE_INSTALL_PREFIX:PATH="C:\Users\test\source\repos\CMakeProject42\out\install\x64-Debug" -DCMAKE_TOOLCHAIN_FILE=F:/Feature-test/cdt/vcpkg/scripts/buildsystems/vcpkg.cmake "C:\Users\test\source\repos\CMakeProject42" 2>&1"
1> Working directory: C:\Users\test\source\repos\CMakeProject42\out\build\x64-Debug
1> [CMake] -- Selecting Windows SDK version 10.0.22000.0 to target Windows 10.0.19045.
1> [CMake] -- Configuring done
1> [CMake] CMake Error at F:/Feature-test/cdt/vcpkg/scripts/buildsystems/vcpkg.cmake:582 (_add_executable):
1> [CMake]   Target "main" links to target "Boost::boost" but the target was not found.
1> [CMake]   Perhaps a find_package() call is missing for an IMPORTED target, or an
1> [CMake]   ALIAS target is missing?
1> [CMake] Call Stack (most recent call first):
1> [CMake]   CMakeProject42/CMakeLists.txt:12 (add_executable)
1> [CMake] -- Generating done
1> [CMake] CMake Generate step failed.  Build files cannot be regenerated correctly.
1> 'C:\WINDOWS\system32\cmd.exe' '/c "%SYSTEMROOT%\System32\chcp.com 65001 >NUL && "C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2019\ENTERPRISE\COMMON7\IDE\COMMONEXTENSIONS\MICROSOFT\CMAKE\CMake\bin\cmake.exe"  -G "Visual Studio 16 2019" -A x64  -DCMAKE_CONFIGURATION_TYPES:STRING="Debug" -DCMAKE_INSTALL_PREFIX:PATH="C:\Users\test\source\repos\CMakeProject42\out\install\x64-Debug" -DCMAKE_TOOLCHAIN_FILE=F:/Feature-test/cdt/vcpkg/scripts/buildsystems/vcpkg.cmake "C:\Users\test\source\repos\CMakeProject42" 2>&1"' execution failed with error: ''C:\WINDOWS\system32\cmd.exe' '/c "%SYSTEMROOT%\System32\chcp.com 65001 >NUL && "C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2019\ENTERPRISE\COMMON7\IDE\COMMONEXTENSIONS\MICROSOFT\CMAKE\CMake\bin\cmake.exe"  -G "Visual Studio 16 2019" -A x64  -DCMAKE_CONFIGURATION_TYPES:STRING="Debug" -DCMAKE_INSTALL_PREFIX:PATH="C:\Users\test\source\repos\CMakeProject42\out\install\x64-Debug" -DCMAKE_TOOLCHAIN_FILE=F:/Feature-test/cdt/vcpkg/scripts/buildsystems/vcpkg.cmake "C:\Users\test\source\repos\CMakeProject42" 2>&1"' returned with exit code: 1'.

@JonLiu1993
Copy link
Member

Pinging @ex-purple for response.

@ex-purple
Copy link
Contributor Author

ex-purple commented Mar 14, 2023

@JonLiu1993
Yes, cdt[boost] depends on boost and I added its to the dependencies. But I don't know how resolve this for the cmake config.

You just can prepend
find_package(Boost CONFIG REQUIRED)
before
find_package(CDT CONFIG REQUIRED)
and all must be fine.

If this should be solved in another way please show me how

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Mar 28, 2023
@vicroms vicroms dismissed BillyONeal’s stale review March 28, 2023 18:41

comments resolved

@vicroms vicroms merged commit 9126276 into microsoft:master Mar 28, 2023
@Nosenzor
Copy link

Thank you for the PR !


if(CDT_USE_BOOST)
- target_link_libraries(${PROJECT_NAME} INTERFACE Boost::boost)
+ target_link_libraries(${PROJECT_NAME} PUBLIC Boost::boost)
Copy link

@artem-ogre artem-ogre Apr 4, 2023

Choose a reason for hiding this comment

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

@ex-purple
Hello, CDT maintainer here.
Could you explain why this patch is necessary? Maybe I could do the change upstream to avoid the need for patching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks this #29324 (comment)
Otherwise the compiler just doesn't know where to find the boost headers to build cdt as library.

Choose a reason for hiding this comment

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

@ex-purple
Sorry, I still don't get it :)
Error in this comment looks to be an include error. As CDT does not require any of the compiled parts of boost, in my understanding using target_link_libraries with INTERFACE should be enough. At least it works for cdt's automated CI builds.

Copy link

@artem-ogre artem-ogre Apr 4, 2023

Choose a reason for hiding this comment

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

When I build CDT as header-only and use PUBLIC I get the CMake error: "INTERFACE library can only be used with the INTERFACE keyword"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look this: https://cmake.org/cmake/help/latest/module/FindBoost.html#imported-targets

Boost::boost
Target for header-only dependencies. (Boost include directory).

The command target_link_libraries(${PROJECT_NAME} PUBLIC Boost::boost) provides path to the boost headers

How else would the compiler find the boost header files to build CDT?

fatal error C1083: Cannot open include file: 'boost/container/flat_set.hpp':

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I build CDT as header-only and use PUBLIC I get the CMake error: "INTERFACE library can only be used with the INTERFACE keyword"

Just check the value of the CDT_USE_AS_COMPILED_LIBRARY flag to choose between the INTERFACE or PUBLIC option.

Copy link
Member

Choose a reason for hiding this comment

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

Error in this comment looks to be an include error. As CDT does not require any of the compiled parts of boost, in my understanding using target_link_libraries with INTERFACE should be enough. At least it works for cdt's automated CI builds.

You are looking to get boost's include directories added to the build, so they need to be in cdt's build. INTERFACE supplies those include directories only to downstream consumers of your library.

You might not see this in your CI if another one of your dependencies happens to add the same include path, or if your system has another copy of boost installed somewhere global like /usr/include that is actually getting used here rather than the copy the user expected.

@artem-ogre
Copy link

Great to see CDT in vcpkg 🥳
Feel free to ping me if I can help with anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Port Request] CDT : Constrained Delaunay Triangulation
6 participants