-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
[au] new port #41119
base: master
Are you sure you want to change the base?
[au] new port #41119
Conversation
e8086cb
to
3cc3b56
Compare
Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags. |
fdff8ad
to
8b16f85
Compare
@JonLiu1993 Thank you for your review. I've gone ahead and opened a PR on the docs site microsoft/vcpkg-docs#405 to include some of the best practices you are showing here. |
Tested usage successfully by
|
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 manifest lacks a license
field.
The patch could be minimized.
-FetchContent_MakeAvailable(googletest) | ||
-include(GoogleTest) |
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.
It is more or less enough to remove those two lines.
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.
I see, so that's the line that does the download, not the declare line.
) | ||
- | ||
- # Add the test, if requested. | ||
- if (DEFINED ARG_GTEST_SRCS) |
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.
Changing this line to if(0)
would be sufficient.
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.
Thanks, patch looks much smaller now
ports/au/portfile.cmake
Outdated
REF "${VERSION}" | ||
SHA512 4aa3282f6b76fbadd04ca572734f72c86b1b0b4e85fc21a03d1ab00b83d3aea319ab2dac3934361b5f6fa7c4a0dccece94fe0a57f3d73d208315b51b1950e374 | ||
HEAD_REF main | ||
PATCHES cmakelists.patch |
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.
Nit-pick:
PATCHES cmakelists.patch | |
PATCHES | |
cmakelists.patch |
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.
yup
ports/au/cmakelists.patch
Outdated
# (This is necessary for it to be usable in other CMake projects.) | ||
|
||
-set(AU_CMAKE_DIR ${CMAKE_INSTALL_LIBDIR}/cmake/Au) | ||
+set(AU_CMAKE_DIR ${CMAKE_INSTALL_DATAROOTDIR}/cmake/Au) |
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.
This would be solved without patching with vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake/Au)
(from the vpckg-cmake-config
host port).
share/cmake/Au
is also wrong, vcpkg wants share/au
. (vcpkg_cmake_config_fixup
would do it right, given the port name.)
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.
Thanks, that works, although I then needed to add
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/lib")
because vcpkg_cmake_config_fixup
left behind an empty lib
folder.
ports/au/portfile.cmake
Outdated
vcpkg_cmake_configure(SOURCE_PATH "${SOURCE_PATH}") | ||
vcpkg_cmake_install() | ||
|
||
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug") |
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.
Wait, is this header-only? Then we don't need to build debug.
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug") |
here, and add the top of the file, add
set(VCPKG_BUILD_TYPE release) # header-only
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.
Looks like this works. 👍🏼
Co-authored-by: JonLiu1993 <63675417+JonLiu1993@users.noreply.github.com>
8b16f85
to
9e1c61c
Compare
Since this port is attempting to take a two letter name, I'm tagging vcpkg-team-review to discuss whether or not there is a need for disambiguation. |
"port-version"
is reset (removed fromvcpkg.json
)../vcpkg x-add-version --all
and committing the result.