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

[mathgl] Fix incorrect target name and incorrect executable path #28315

Closed

Conversation

LilyWangLL
Copy link
Contributor

Describe the pull request

@LilyWangLL LilyWangLL added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Dec 13, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 13, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 13, 2022
@LilyWangLL LilyWangLL marked this pull request as ready for review December 13, 2022 13:52
@dg0yt
Copy link
Contributor

dg0yt commented Dec 13, 2022

This overlaps with #28311.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 13, 2022

PS: Test non-default feautures, CI won't do it for us.

@LilyWangLL LilyWangLL added the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label Dec 14, 2022
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

I hope I won't be forced to rebase my PR on a revert of these changes.

Comment on lines +59 to +62
vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/share/MathGL2/MathGLTargets-debug.cmake"
"${VCPKG_IMPORT_PREFIX}/debug/${PATH}/${TOOL_NAME}.exe" "${VCPKG_IMPORT_PREFIX}/tools/mathgl/${TOOL_NAME}.exe")
vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/share/MathGL2/MathGLTargets-release.cmake"
"${VCPKG_IMPORT_PREFIX}/${PATH}/${TOOL_NAME}.exe" "${VCPKG_IMPORT_PREFIX}/tools/mathgl/${TOOL_NAME}.exe")
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this is not the right fix. Normally, this should be handled by vcpkg_cmake_config_fixup. If it doesn't work, it might need a fix there.

Copy link
Contributor

@JackBoosY JackBoosY Dec 14, 2022

Choose a reason for hiding this comment

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

This action should be run before calling vcpkg_cmake_config_fixup to avoid executable target path is changed.
And we should use vcpkg_copy_tools since we should know which tools should be installed.

Copy link
Contributor Author

@LilyWangLL LilyWangLL Dec 14, 2022

Choose a reason for hiding this comment

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

I use vcpkg_copy_tools to handle tools before calling vcpkg_cmake_config_fixup, but the tools path in MathGLTargets-debug.cmake is still incorrect.

Copy link
Contributor

Choose a reason for hiding this comment

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

I use vcpkg_copy_tools to handle tools before calling vcpkg_cmake_config_fixup, but the tools path in MathGLTargets-debug.cmake is still incorrect.

Ah, I think you may need to make a patch to fix the tool installation path?

Copy link
Contributor

Choose a reason for hiding this comment

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

#28311 is more complete. CI testing of widgets components showed more problems. That's why it is still in progress.

@Cheney-W Cheney-W marked this pull request as draft December 20, 2022 09:32
@LilyWangLL
Copy link
Contributor Author

Closed as duplicate of #28311.

@LilyWangLL LilyWangLL closed this Jan 3, 2023
@LilyWangLL LilyWangLL deleted the dev/LilyWang/issue28285 branch January 17, 2023 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could not find mathglConfig.cmake CMake not able to find MathGL
4 participants