[aurora-au] new port by andre-nguyen · Pull Request #41119 · microsoft/vcpkg

andre-nguyen · 2024-09-22T18:01:38Z

Changes comply with the maintainer guide.
SHA512s are updated for each updated download.
The "supports" clause reflects platforms that may be fixed by this new version.
Any fixed CI baseline entries are removed from that file.
Any patches that are no longer applied are deleted from the port's directory.
When updating the upstream version, the "port-version" is reset (removed from vcpkg.json).
The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
Only one version is added to each modified port's versions file.

JonLiu1993 · 2024-09-23T07:24:54Z

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.

andre-nguyen · 2024-09-23T12:57:58Z

@JonLiu1993 Thank you for your review. I've gone ahead and opened a PR on the docs site MicrosoftDocs/vcpkg-docs#405 to include some of the best practices you are showing here.

JonLiu1993 · 2024-09-24T02:35:52Z

Tested usage successfully by au:x64-windows:

au is header-only and can be used from CMake via:

  find_path(AU_INCLUDE_DIRS "au/apply_magnitude.hh")
  target_include_directories(main PRIVATE ${AU_INCLUDE_DIRS})

dg0yt

The manifest lacks a license field.

The patch could be minimized.

dg0yt · 2024-09-24T06:41:30Z

+-FetchContent_MakeAvailable(googletest)
+-include(GoogleTest)


It is more or less enough to remove those two lines.

I see, so that's the line that does the download, not the declare line.

dg0yt · 2024-09-24T06:43:29Z

+   )
+-
+-  # Add the test, if requested.
+-  if (DEFINED ARG_GTEST_SRCS)


Changing this line to if(0) would be sufficient.

Thanks, patch looks much smaller now

dg0yt · 2024-09-24T06:44:23Z

+    REF "${VERSION}"
+    SHA512 4aa3282f6b76fbadd04ca572734f72c86b1b0b4e85fc21a03d1ab00b83d3aea319ab2dac3934361b5f6fa7c4a0dccece94fe0a57f3d73d208315b51b1950e374
+    HEAD_REF main
+    PATCHES cmakelists.patch


Nit-pick:

Suggested change

PATCHES cmakelists.patch

PATCHES

cmakelists.patch

dg0yt · 2024-09-24T06:49:53Z

+ # (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)


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

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.

dg0yt · 2024-09-24T06:51:20Z

+vcpkg_cmake_configure(SOURCE_PATH "${SOURCE_PATH}")
+vcpkg_cmake_install()
+
+file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug")


Wait, is this header-only? Then we don't need to build debug.

Suggested change

file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug")

here, and add the top of the file, add

set(VCPKG_BUILD_TYPE release) # header-only

Looks like this works. 👍🏼

Co-authored-by: JonLiu1993 <63675417+JonLiu1993@users.noreply.github.com>

JavierMatosD · 2024-09-25T19:19:33Z

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.

andre-nguyen · 2024-10-02T17:24:07Z

@JavierMatosD just wondering, what is typically the review turnaround time?
Not urgent, just wondering.

JavierMatosD · 2024-10-03T22:21:53Z

@ras0219-msft, @data-queue, @BillyONeal, @AugP and I discussed this today.

@andre-nguyen, we ask that you prepend 'Aurora-' to the port name (aurora-au) to disambiguate the port name.

@chiphogg, + courtesy ping

JavierMatosD · 2024-10-03T22:39:58Z

@JavierMatosD just wondering, what is typically the review turnaround time? Not urgent, just wondering.

We typically hold weekly team meetings to review PRs that need more detailed attention. However, we often receive more PRs than we can reasonably review in a week, so things may occasionally pile up. While there isn’t a “typical” turnaround time, we’re actively working on streamlining the review process to improve the experience for everyone.

chiphogg

Wow, this is really exciting!

Independently, other contributors have started the process of bringing Au into conan. That process is suggesting some CMake changes that I think would help the vcpkg port as well, namely, making it easier to avoid the googletest dependency natively without patching.

I'm excited to see the library reach a wider audience, and grateful for the support!

andre-nguyen · 2024-10-08T20:34:41Z

Hello @JonLiu1993, just want to check in and see if you are aware the PR has been updated and is ready for another review.

JonLiu1993 · 2024-10-09T02:55:57Z

@andre-nguyen, When I tested the usage by aurora-au:x64-windows, I get this error, please take a look:

1> [CMake] CMake Error at F:/Feature-test/aurora-au/scripts/buildsystems/vcpkg.cmake:859 (_find_package):
1> [CMake]   By not providing "Findgoogletest.cmake" in CMAKE_MODULE_PATH this project
1> [CMake]   has asked CMake to find a package configuration file provided by
1> [CMake]   "googletest", but CMake did not find one.
1> [CMake] 
1> [CMake]   Could not find a package configuration file provided by "googletest"
1> [CMake]   (requested version 1.12.1) with any of the following names:
1> [CMake] 
1> [CMake]     googletestConfig.cmake
1> [CMake]     googletest-config.cmake
1> [CMake] 
1> [CMake]   Add the installation prefix of "googletest" to CMAKE_PREFIX_PATH or set
1> [CMake]   "googletest_DIR" to a directory containing one of the above files.  If
1> [CMake]   "googletest" provides a separate development package or SDK, be sure it has
1> [CMake]   been installed.
1> [CMake] Call Stack (most recent call first):
1> [CMake]   C:/Program Files/Microsoft Visual Studio/2022/Community/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.29/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
1> [CMake]   F:/Feature-test/aurora-au/installed/x64-windows/share/aurora-au/AuConfig.cmake:42 (find_dependency)
1> [CMake]   F:/Feature-test/aurora-au/scripts/buildsystems/vcpkg.cmake:859 (_find_package)
1> [CMake]   CMakeProject1/CMakeLists.txt:6 (find_package)
1> [CMake] -- Configuring incomplete, errors occurred!

chiphogg · 2024-10-09T14:31:42Z

@andre-nguyen, When I tested the usage by aurora-au:x64-windows, I get this error, please take a look:

1> [CMake] CMake Error at F:/Feature-test/aurora-au/scripts/buildsystems/vcpkg.cmake:859 (_find_package):
1> [CMake]   By not providing "Findgoogletest.cmake" in CMAKE_MODULE_PATH this project
1> [CMake]   has asked CMake to find a package configuration file provided by
1> [CMake]   "googletest", but CMake did not find one.
1> [CMake] 
1> [CMake]   Could not find a package configuration file provided by "googletest"
1> [CMake]   (requested version 1.12.1) with any of the following names:
1> [CMake] 
1> [CMake]     googletestConfig.cmake
1> [CMake]     googletest-config.cmake
1> [CMake] 
1> [CMake]   Add the installation prefix of "googletest" to CMAKE_PREFIX_PATH or set
1> [CMake]   "googletest_DIR" to a directory containing one of the above files.  If
1> [CMake]   "googletest" provides a separate development package or SDK, be sure it has
1> [CMake]   been installed.
1> [CMake] Call Stack (most recent call first):
1> [CMake]   C:/Program Files/Microsoft Visual Studio/2022/Community/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.29/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
1> [CMake]   F:/Feature-test/aurora-au/installed/x64-windows/share/aurora-au/AuConfig.cmake:42 (find_dependency)
1> [CMake]   F:/Feature-test/aurora-au/scripts/buildsystems/vcpkg.cmake:859 (_find_package)
1> [CMake]   CMakeProject1/CMakeLists.txt:6 (find_package)
1> [CMake] -- Configuring incomplete, errors occurred!

Possible places to check for this:

dg0yt · 2024-10-09T16:59:14Z

There is a patch to disable the googletest dependency on port build.
This patch must be extended to remove the find_dependency(googletest).

I would also suggest to rename the patch to disable-googletest.diff or similar.

andre-nguyen · 2024-10-10T14:41:19Z

Thank you everyone for the tips.
Seems to be working on my end now with a small test project.

I have renamed the patch file to disable-googletest.patch
Added a usage file

JonLiu1993 · 2024-10-11T09:11:20Z

Tested usage successfully by aurora-au:x64-windows:

The package aurora-au provides CMake targets:

    find_package(Au REQUIRED)
    target_link_libraries(main PRIVATE Au::au)

chiphogg · 2024-10-21T19:12:20Z

Thanks @andre-nguyen for adding this!

When I look at the official entry on vcpkg, it says "Documentation: Unavailable". We actually have a full doc website, for both the listed version 0.3.5, and for head. How hard would it be to add these links?

andre-nguyen · 2024-10-22T18:57:16Z

Thanks @andre-nguyen for adding this!

When I look at the official entry on vcpkg, it says "Documentation: Unavailable". We actually have a full doc website, for both the listed version 0.3.5, and for head. How hard would it be to add these links?

Looks fairly straightforward, its just a documentation field in the json file. I can do it later.

chiphogg · 2024-12-12T17:09:39Z

@andre-nguyen, would you be able/interested in updating to Au 0.4.0? It's out now, and it should avoid the need for a lot of the CMake workarounds!

(I thought I'd ask you first because I've never used vcpkg, never put together a vcpkg PR, and haven't signed the CLA.)

JonLiu1993 · 2024-12-13T03:10:18Z

@andre-nguyen, would you be able/interested in updating to Au 0.4.0? It's out now, and it should avoid the need for a lot of the CMake workarounds!

(I thought I'd ask you first because I've never used vcpkg, never put together a vcpkg PR, and haven't signed the CLA.)

I commit a PR #42679 update aurora-au to the 0.4.0

andre-nguyen force-pushed the port/au branch 2 times, most recently from e8086cb to 3cc3b56 Compare September 22, 2024 18:42

andre-nguyen marked this pull request as ready for review September 22, 2024 18:52

andre-nguyen changed the title ~~draft: [au] new port~~ [au] new port Sep 22, 2024

LilyWangLL assigned JonLiu1993 Sep 23, 2024

JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Sep 23, 2024