-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[lib3mf] New port. Introduces version 2.3.2 #39886
Conversation
lib3mf VCPKG port
@microsoft-github-policy-service agree company="3MF Consortium" |
@dg0yt I have made the following changes
I will discuss and implement your other suggestions in the upstream project in future releases. |
@WangWeiLin-MV Could you please let me know if all the issues have been resolved? Thanks. |
Co-authored-by: WangWeiLin-MV <[email protected]>
@WangWeiLin-MV I have committed the requested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port usage tests pass with the following triplets:
x64-linuxx64-windows
Invalid check after update.
Co-authored-by: WangWeiLin-MV <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port usage tests pass with the following triplets:
- x64-linux
- x64-windows
@WangWeiLin-MV @dg0yt Thank you so much for valuable time and comments. |
ports/lib3mf/lib3mf_vcpkg.patch
Outdated
+ find_dependency(libzip) | ||
+ find_dependency(ZLIB) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here these are controlled by VCPKG_TOOLCHAIN
but elsewhere they're controlled by separate options. What controls find_dependency
should always match what controls it in the CMakeLists.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As written the patch seems inapplicable for either the goal of minimizing the size of the patch in vcpkg, as well as the goal of matching exactly what upstream should do. To that end, I would like to see something like one of the following.
Option A: Minimize the size of the patch to only what vcpkg needs to touch, I've submitted as 3MFConsortium#2
Option B: Actually wire up the new options correctly with respect to what upstream needs to do to be correct in their CMake configs; I've submitted that as 3MFConsortium#3
Reduce the size of the patch by removing optionality vcpkg does not use.
@BillyONeal Thank you so much for taking the time. I have accepted your Option A. Kindly check and let me know if its good enough to be approved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port usage tests pass with the following triplets:
- x64-linux
- x64-windows
Thanks for the new port! |
This PR introduces a new port for lib3mf. The version is 2.3.2
find_package
calls are REQUIRED, are satisfied byvcpkg.json
's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.vcpkg.json
matches what upstream says.vcpkg.json
matches what upstream says../vcpkg x-add-version --all
and committing the result.