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

[small-gicp] New port #39393

Merged
merged 10 commits into from
Jun 27, 2024
Merged

Conversation

andre-nguyen
Copy link
Contributor

@andre-nguyen andre-nguyen commented Jun 19, 2024

If this PR adds a new port, please uncomment and fill out this checklist:

  • 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 adding-usage 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.

@andre-nguyen andre-nguyen marked this pull request as draft June 19, 2024 19:51
@MonicaLiu0311 MonicaLiu0311 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jun 20, 2024
@MonicaLiu0311
Copy link
Contributor

Please get failure logs for x86-windows here:

[1/4] C:\PROGRA~1\MICROS~1\2022\ENTERP~1\VC\Tools\MSVC\1438~1.331\bin\Hostx64\x86\cl.exe   /TP -D_USE_MATH_DEFINES -Dsmall_gicp_EXPORTS -ID:\b\small-gicp\src\v0.1.0-8bc82f4870.clean\include -external:ID:\installed\x86-windows\include\eigen3 -external:W0 /nologo /DWIN32 /D_WINDOWS /W3 /utf-8 /GR /EHsc /MP  /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1  -std:c++17 -MDd -openmp /showIncludes /FoCMakeFiles\small_gicp.dir\src\small_gicp\registration\registration_helper.cpp.obj /FdCMakeFiles\small_gicp.dir\ /FS -c D:\b\small-gicp\src\v0.1.0-8bc82f4870.clean\src\small_gicp\registration\registration_helper.cpp
FAILED: CMakeFiles/small_gicp.dir/src/small_gicp/registration/registration_helper.cpp.obj 
C:\PROGRA~1\MICROS~1\2022\ENTERP~1\VC\Tools\MSVC\1438~1.331\bin\Hostx64\x86\cl.exe   /TP -D_USE_MATH_DEFINES -Dsmall_gicp_EXPORTS -ID:\b\small-gicp\src\v0.1.0-8bc82f4870.clean\include -external:ID:\installed\x86-windows\include\eigen3 -external:W0 /nologo /DWIN32 /D_WINDOWS /W3 /utf-8 /GR /EHsc /MP  /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1  -std:c++17 -MDd -openmp /showIncludes /FoCMakeFiles\small_gicp.dir\src\small_gicp\registration\registration_helper.cpp.obj /FdCMakeFiles\small_gicp.dir\ /FS -c D:\b\small-gicp\src\v0.1.0-8bc82f4870.clean\src\small_gicp\registration\registration_helper.cpp
D:\b\small-gicp\src\v0.1.0-8bc82f4870.clean\include\small_gicp/ann/incremental_voxelmap.hpp(148): error C2338: static_assert failed: 'size_t must be 64-bit'
D:\b\small-gicp\src\v0.1.0-8bc82f4870.clean\include\small_gicp/ann/incremental_voxelmap.hpp(148): note: the template instantiation context (the oldest one first) is
D:\b\small-gicp\src\v0.1.0-8bc82f4870.clean\include\small_gicp/ann/incremental_voxelmap.hpp(160): note: see reference to class template instantiation 'small_gicp::IncrementalVoxelMap<VoxelContents>' being compiled
warning: Task-based OpenMP parallelism causes run-time memory errors with Eigen matrices.
warning: Thus, OpenMP-based multi-threading for KdTree construction is disabled on MSVC.
warning: Task-based OpenMP parallelism is not well supported on windows.

Please get failure logs for x64-osx here:

CMake Error at /usr/local/Cellar/cmake/3.29.3/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find OpenMP_C (missing: OpenMP_C_FLAGS OpenMP_C_LIB_NAMES)
Call Stack (most recent call first):
  /usr/local/Cellar/cmake/3.29.3/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  /usr/local/Cellar/cmake/3.29.3/share/cmake/Modules/FindOpenMP.cmake:581 (find_package_handle_standard_args)
  /Users/vcpkg/Data/work/2/s/scripts/buildsystems/vcpkg.cmake:859 (_find_package)
  CMakeLists.txt:62 (find_package)


-- Configuring incomplete, errors occurred!

@MonicaLiu0311 MonicaLiu0311 changed the title Add small_gicp port [small_gicp] New port Jun 20, 2024
@andre-nguyen andre-nguyen marked this pull request as ready for review June 21, 2024 16:39
@MonicaLiu0311
Copy link
Contributor

All features are tested successfully in the following triplet:

x64-windows
x64-windows-static

The usage test passed on x64-windows (header files found):

small-gicp provides CMake targets:

  find_package(small_gicp REQUIRED)
  target_link_libraries(main PRIVATE small_gicp::small_gicp)

@andre-nguyen
Copy link
Contributor Author

Thank you @MonicaLiu0311, I have addressed the review comments.

@MonicaLiu0311
Copy link
Contributor

At present, the version information of your commit is inconsistent with your local one:

##[error]Found the following errors:
##[error]versions/s-/small-gicp.json(2,): error : while parsing versions for small-gicp from version/s-/small-gicp.json
0.1.0 is declared with 9100c3029150c44f0c3b2c7cc6ea1a861de90587, but the local port has a different SHA 580a9b893d2996aceb7ed24d6fcb0bc2111f0553.

Please run ./vcpkg x-add-version testPort [--overwrite-version] before each push:

  1. For any file modification under the ports/testPort/ folder, please commit after modification
  2. Run ./vcpkg x-add-version testPort to generate a series of version according to your commit info (Add --overwrite-version when generating other than the first time)
  3. Commit verison information again
  4. Push together

--overwrite-version
After the first modification, please run ./vcpkg x-add-version testPort to generate the corresponding json; for subsequent modifications of the same version, please use ./vcpkg x-add-version portName --overwrite-version to update git-tree.

maintainer guide#versioning.

@MonicaLiu0311
Copy link
Contributor

Done.
image

@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Jun 25, 2024
@andre-nguyen
Copy link
Contributor Author

Ah, thank you for that. So just to confirm, basically any time I make a change, I also need to rerun vcpkg x-add-version ... ?

@MonicaLiu0311
Copy link
Contributor

Ah, thank you for that. So just to confirm, basically any time I make a change, I also need to rerun vcpkg x-add-version ... ?

Yes, you need to regenerate new version information before each push. These version information are generated based on your latest commit (modifications in the port/testPort/ directory).

@andre-nguyen andre-nguyen requested a review from valgur June 26, 2024 15:30
@valgur
Copy link
Contributor

valgur commented Jun 26, 2024

The AppleClang OpenMP issue is actually due to a bug I introduced, it appears. Sorry about that!

Use this patch instead:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -22,7 +22,7 @@
 option(ENABLE_COVERAGE "Enable coverage" OFF)
 
 # Dependency options
-set(BUILD_WITH_OPENMP CACHE STRING "Build with OpenMP" "auto")
+set(BUILD_WITH_OPENMP "auto" CACHE STRING "Build with OpenMP")
 option(BUILD_WITH_TBB "Build with TBB" OFF)
 option(BUILD_WITH_PCL "Build with PCL (required for benchmark and test only)" OFF)
 option(BUILD_WITH_FAST_GICP "Build with fast_gicp (required for benchmark and test only)" OFF)

I opened a PR as well: koide3/small_gicp#76

@BillyONeal BillyONeal changed the title [small_gicp] New port [small-gicp] New port Jun 27, 2024
@BillyONeal BillyONeal merged commit 85782ff into microsoft:master Jun 27, 2024
17 checks passed
@BillyONeal
Copy link
Member

Thanks for the new port!

@andre-nguyen
Copy link
Contributor Author

@valgur I'll update with the fix when @koide3 releases a new tag/version

@koide3
Copy link

koide3 commented Jun 28, 2024

I just pushed a new tag v.0.1.2 with @valgur 's fix.

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.

5 participants