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

Add manifold port #42347

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

JeffreyWardman
Copy link

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

@JeffreyWardman
Copy link
Author

@microsoft-github-policy-service agree

@JeffreyWardman
Copy link
Author

I was able to build locally. manifold builds files into lib/cmake which raises warnings, which seem to have caused the CI/CD to fail. Once adding the ignore warnings, I get the current errors only in CI/CD.

Initial error due to warnings: https://dev.azure.com/vcpkg/public/_build/results?buildId=109648&view=logs&j=878666d5-db33-5b27-9e7d-b0c7ee352005&t=69176ca8-c93b-5311-0f74-6cb12a2b3f92&l=8209

After adding the ignore fixes: https://dev.azure.com/vcpkg/public/_build/results?buildId=109649&view=logs&j=878666d5-db33-5b27-9e7d-b0c7ee352005&t=69176ca8-c93b-5311-0f74-6cb12a2b3f92&l=8184

This is my first commit to vcpkg. It'd be great if I can get advice on how to handle these kinds of warnings for it to be merged.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

This is my first commit to vcpkg. It'd be great if I can get advice on how to handle these kinds of warnings for it to be merged.

Usually these kinds of problems require patching upstream's build system, or moving around bits in portfile.cmake.

Do you have specific questions about the warnings? They seem pretty explicit in what the problem is to me, but then again, I wrote most of the messages 😅

Repeating the errors here so that the logs aren't lost when the build gets purged in a few days:

D:\a_work\1\s\ports\manifold\portfile.cmake: warning: ${CURRENT_PACKAGES_DIR}/debug/include should not exist. To suppress this message, add set(VCPKG_POLICY_ALLOW_DEBUG_INCLUDE enabled)
note: If this directory was created by a build system that does not allow installing headers in debug to be disabled, delete the duplicate directory with file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/include")
D:\a_work\1\s\ports\manifold\portfile.cmake: warning: This port installs the following CMake files in places CMake files are not expected. CMake files should be installed in ${CURRENT_PACKAGES_DIR}/share/${PORT}. To suppress this message, add set(VCPKG_POLICY_SKIP_MISPLACED_CMAKE_FILES_CHECK enabled)
D:\p\manifold_x86-windows: note: the files are relative to ${CURRENT_PACKAGES_DIR} here
note: lib/cmake/manifold/manifoldConfig.cmake
note: lib/cmake/manifold/manifoldConfigVersion.cmake
note: lib/cmake/manifold/manifoldTargets-release.cmake
note: lib/cmake/manifold/manifoldTargets.cmake
note: debug/lib/cmake/manifold/manifoldConfig.cmake
note: debug/lib/cmake/manifold/manifoldConfigVersion.cmake
note: debug/lib/cmake/manifold/manifoldTargets-debug.cmake
note: debug/lib/cmake/manifold/manifoldTargets.cmake
D:\a_work\1\s\ports\manifold\portfile.cmake: warning: This port creates ${CURRENT_PACKAGES_DIR}/lib/cmake and/or ${CURRENT_PACKAGES_DIR}/debug/lib/cmake, which should be merged and moved to ${CURRENT_PACKAGES_DIR}/share/${PORT}/cmake. Please use the helper function vcpkg_cmake_config_fixup() from the port vcpkg-cmake-config. To suppress this message, add set(VCPKG_POLICY_SKIP_LIB_CMAKE_MERGE_CHECK enabled)
D:\a_work\1\s\ports\manifold\portfile.cmake: warning: Found 3 post-build check problem(s). These are usually caused by bugs in portfile.cmake or the upstream build system. Please correct these before submitting this port to the curated registry.
error: building manifold:x86-windows failed with: POST_BUILD_CHECKS_FAILED

Comment on lines 1 to 3
add set(VCPKG_POLICY_ALLOW_DEBUG_INCLUDE enabled)
set(VCPKG_POLICY_SKIP_MISPLACED_CMAKE_FILES_CHECK enabled)
add set(VCPKG_POLICY_SKIP_LIB_CMAKE_MERGE_CHECK enabled)
Copy link
Member

Choose a reason for hiding this comment

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

We can't merge ports which fail these kinds of post-build checks into the curated registry. You will need to fix these problems in the build system.

@JeffreyWardman
Copy link
Author

The warnings are very clear to me! I'll fix the issues in the next day or two

@FrankXie05 FrankXie05 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Nov 26, 2024
ports/manifold/portfile.cmake Outdated Show resolved Hide resolved
ports/manifold/portfile.cmake Outdated Show resolved Hide resolved
ports/manifold/portfile.cmake Outdated Show resolved Hide resolved
ports/manifold/portfile.cmake Outdated Show resolved Hide resolved
ports/manifold/portfile.cmake Outdated Show resolved Hide resolved
ports/manifold/portfile.cmake Outdated Show resolved Hide resolved
ports/manifold/portfile.cmake Show resolved Hide resolved
ports/manifold/usage Outdated Show resolved Hide resolved
@JeffreyWardman
Copy link
Author

Waiting for CI/CD pipeline to complete but I expect that I need to add:

  "supports": "((x64 & (windows | osx | linux)) | (x86 & windows) | (arm64 & osx)) & !uwp & !static",

@JeffreyWardman
Copy link
Author

JeffreyWardman commented Nov 26, 2024

So, it supports:
x86_windows, x64_windows, x64_windows_static_md, x64_uwp, arm64_windows, arm64_windows_static_md, arm64_uwp, x64_osx, arm64_osx, x64_linux.

It does not support x64_windows_static, arm_neon_android, x64_android, arm64_android.

How can I represent this? What do I extend onto "supports": "!android" to include x64_windows_static only?

@dg0yt
Copy link
Contributor

dg0yt commented Nov 27, 2024

The failures for android and x64-windows-static are due to vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY) in triplets with static crt linkage. This shall not be reflected in supports but as expected fail in script/ci.baseline.txt. (User can overcome the guard via per-port customization.)

@JeffreyWardman JeffreyWardman marked this pull request as draft November 27, 2024 00:53
@JeffreyWardman JeffreyWardman marked this pull request as ready for review November 27, 2024 01:04
@JeffreyWardman JeffreyWardman marked this pull request as draft November 27, 2024 01:20
@JeffreyWardman JeffreyWardman marked this pull request as ready for review November 27, 2024 02:09
@JeffreyWardman
Copy link
Author

Thanks so much for your review and help so far @dg0yt and @BillyONeal. I've added the requested changes and removed the (unneeded) dynamic lib check.

All CI/CD pipelines pass if built in release mode but there is an error when compiling for arm-neon-android in debug mode only (an issue on manifold's end). I've created a PR for the fix: elalish/manifold#1077. As this is their 3.0 release, it'd be good to push up the current version.

I've reverted enabling debug builds with the idea that the initial port gets pushed with v3.0.0 and a ticket is created for enabling debug builds that's dependent on the next patch.

Please let me know if an alternative is preferred.

@JeffreyWardman
Copy link
Author

JeffreyWardman commented Nov 27, 2024

Thanks so much for your review and help so far @dg0yt and @BillyONeal. I've added the requested changes and removed the (unneeded) dynamic lib check.

All CI/CD pipelines pass if built in release mode but there is an error when compiling for arm-neon-android in debug mode only (an issue on manifold's end). I've created a PR for the fix: elalish/manifold#1077 and it has been merged (but not as part of the 3.0 release). As this is their 3.0 release, it'd be good to push up the current version.

I've reverted enabling debug builds with the idea that the initial port gets pushed with v3.0.0 and a ticket is created for enabling debug builds that's dependent on the next patch.

Please let me know if an alternative is preferred.

ports/manifold/usage Outdated Show resolved Hide resolved
ports/manifold/portfile.cmake Outdated Show resolved Hide resolved
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants