[dstorage] Add port for Microsoft.Direct3D.DirectStorage NuGet by walbourn · Pull Request #24063 · microsoft/vcpkg

walbourn · 2022-04-09T22:43:52Z

This adds a new port for the DirectStorage API using the NuGet package.

Includes CMake find_package support using a config file.

This is the official vcpkg port being created in coordination with the owner of the Microsoft.Direct3D.DirectStorage nuget package owner.

github-actions

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated

git add -u && git commit
git checkout bd1ef2df46303989eeb048eb7aa9b816aa46365e -- versions
./vcpkg x-add-version --all

Diff

diff --git a/versions/baseline.json b/versions/baseline.json
index c7016f8..67506e5 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -1944,6 +1944,10 @@
       "baseline": "1.7.5",
       "port-version": 0
     },
+    "dstorage": {
+      "baseline": "1.0.0",
+      "port-version": 0
+    },
     "dtl": {
       "baseline": "1.19",
       "port-version": 1

walbourn · 2022-04-09T22:49:19Z

Validated all supported triplets:

vcpkg install dstorage:x86-windows
vcpkg install dstorage:x64-windows
vcpkg install dstorage:arm-windows
vcpkg install dstorage:arm64-windows

ports/dstorage/vcpkg.json

strega-nil-ms

Since this is not an official config.cmake, it needs to be in the unofficial namespace.

Also, probably should lowercase the Config.cmake.in name.

ports/dstorage/Config.cmake.in

coopp · 2022-04-11T19:59:42Z

I am the owner of the Microsoft.Direct3D.DirectStorage nuget package and I agree that this is an official port.

strega-nil · 2022-04-11T22:47:36Z

@coopp alright; what is your thought on Microsoft::DirectStorage? I would personally expect something like Direct3D::DirectStorage (project :: library)

strega-nil · 2022-04-11T22:48:53Z

I also think the following changes should be made:

From 6dbded87ea8f0eb38b9bfda2605f26998abd7726 Mon Sep 17 00:00:00 2001
From: nicole mazzuca <mazzucan@outlook.com>
Date: Mon, 11 Apr 2022 15:48:11 -0700
Subject: [PATCH] minor CR changes

---
 .../dstorage/{Config.cmake.in => dstorage-config.cmake.in}  | 0
 ports/dstorage/portfile.cmake                               | 6 +++---
 2 files changed, 3 insertions(+), 3 deletions(-)
 rename ports/dstorage/{Config.cmake.in => dstorage-config.cmake.in} (100%)

diff --git a/ports/dstorage/Config.cmake.in b/ports/dstorage/dstorage-config.cmake.in
similarity index 100%
rename from ports/dstorage/Config.cmake.in
rename to ports/dstorage/dstorage-config.cmake.in
diff --git a/ports/dstorage/portfile.cmake b/ports/dstorage/portfile.cmake
index e74aaaad6..b8e4ff394 100644
--- a/ports/dstorage/portfile.cmake
+++ b/ports/dstorage/portfile.cmake
@@ -12,8 +12,8 @@ vcpkg_extract_source_archive_ex(
     NO_REMOVE_ONE_LEVEL
 )
 
-file(GLOB HEADER_FILES "${PACKAGE_PATH}/include/DirectStorage/*.h")
-file(INSTALL ${HEADER_FILES} DESTINATION "${CURRENT_PACKAGES_DIR}/include")
+file(INSTALL "${PACKAGE_PATH}/include/DirectStorage"
+    DESTINATION "${CURRENT_PACKAGES_DIR}/include")
 
 file(INSTALL "${PACKAGE_PATH}/bin/${VCPKG_TARGET_ARCHITECTURE}/dstorage.lib" DESTINATION "${CURRENT_PACKAGES_DIR}/lib")
 
@@ -25,4 +25,4 @@ file(COPY "${CURRENT_PACKAGES_DIR}/bin" "${CURRENT_PACKAGES_DIR}/lib" DESTINATIO
 
 file(INSTALL "${PACKAGE_PATH}/LICENSE.txt" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}" RENAME copyright)
 
-configure_file("${CMAKE_CURRENT_LIST_DIR}/Config.cmake.in" "${CURRENT_PACKAGES_DIR}/share/${PORT}/${PORT}-config.cmake" COPYONLY)
+configure_file("${CMAKE_CURRENT_LIST_DIR}/dstorage-config.cmake.in" "${CURRENT_PACKAGES_DIR}/share/${PORT}/${PORT}-config.cmake" COPYONLY)
-- 
2.32.0 (Apple Git-132)

walbourn · 2022-04-11T22:51:19Z

@coopp alright; what is your thought on Microsoft::DirectStorage? I would personally expect something like Direct3D::DirectStorage (project :: library)

I believe the reason the NuGet namespace is Microsoft.Direct3D.* is because how nuget.org manages their namespace ownership within the company. There's not going to be a conflict with ANOTHER Microsoft technology called "DirectStorage", and I believe it's a lot clearer this is a "Microsoft" technology with the current namespace.

find_package(dstorage CONFIG REQUIRED)
target_link_libraries(main PRIVATE Microsoft::DirectStorage)

walbourn · 2022-04-11T23:08:15Z

I also think the following changes should be made:

I didn't use exactly those diffs, but I believe my latest revision should resolve this. Thanks!

strega-nil · 2022-04-11T23:28:34Z

@walbourn I agree that it's unlikely another MS technology is going to compete with the name DirectStorage, in general CMake targets are written project :: package, not company :: package - for example, abseil is not google::abseil, it's absl::blah.

ports/dstorage/portfile.cmake

walbourn · 2022-04-11T23:30:33Z

@walbourn I agree that it's unlikely another MS technology is going to compete with the name DirectStorage, in general CMake targets are written project :: package, not company :: package - for example, abseil is not google::abseil, it's absl::blah.

We already have Microsoft::directxtk, Microsoft::directxmath, etc. Renaming ALL these other ports to Direct3D:: seems far more disruptive than calling this one Microsoft::.

There's also Microsoft::xaudio2redist which has nothing at all to do with Direct3D.

This port is not for an open-source project. It's for a Microsoft closed-source technology and making it available to CMake users.

strega-nil · 2022-04-11T23:39:01Z

alright... I really don't like that that's the way we've done that there, but I'll merge once you've done the install into the correct include directory. Thanks for contributing!

walbourn · 2022-04-11T23:40:00Z

alright... I really don't like that that's the way we've done that there, but I'll merge once you've done the install into the correct include directory. Thanks for contributing!

Won't I also need to change the .in to match the new lower include folder?

strega-nil · 2022-04-11T23:54:54Z

@walbourn the idea is that someone links to Microsoft::DirectStorage, and then includes one of those headers via #include <DirectStorage/dstorage.h> rather than #include <dstorage.h>; if you really want to make it possible to #include <dstorage.h>, you can add both as include directories so that both are allowed (at least, that's my opinion)

walbourn · 2022-04-12T00:08:40Z

@walbourn the idea is that someone links to Microsoft::DirectStorage, and then includes one of those headers via #include <DirectStorage/dstorage.h> rather than #include <dstorage.h>; if you really want to make it possible to #include <dstorage.h>, you can add both as include directories so that both are allowed (at least, that's my opinion)

Clients of the nuget package use #include <dstorage.h>. They can't use anything else.

  <ItemDefinitionGroup>
    <ClCompile>
      <AdditionalIncludeDirectories>$(MSBuildThisFileDirectory)..\Include\DirectStorage;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
    </ClCompile>
  </ItemDefinitionGroup>

Also, as I said above, the next revision of the NuGet package is going to eliminate the include\DirectStorage subfolder and just use include which is more consistent with other NuGet packages.

IOW: If I followed your suggestion, then it will break client usage in the next port revision: #include <DirectStorage/dstorage.h> won't work.

strega-nil · 2022-04-12T00:28:06Z

mhh, alright; then, merging as is. Thanks for talking it out with me!

walbourn · 2022-04-12T00:34:49Z

Thanks!

I'm happy to revisit the taxonomy of namespaces for my Microsoft org hosted projects directxtk, directxtk12, directxtex, directxmesh, uvatlas, and directxmath if Microsoft:: is inappropriate. Maybe I could use DirectX::?

I'm not sure there is any better namespace than Microsoft::, however, for xaudio2redist, dxsdk-d3dx, and dstorage since they are just raw projections of nuget packages for closed-source binaries.

[dstorage] Add port for Microsoft.Direct3D.DirectStorage NuGet (microsoft#24063)

* master: (139 commits) [dstorage] Add port for Microsoft.Direct3D.DirectStorage NuGet (microsoft#24063) [vcpkg] Refactor toolchain & generator selection (microsoft#23846) [icu] update to 70.1 (microsoft#23445) [vcpkg] Update android usage documentation (microsoft#23690) [LMDB] update to 0.9.29 (microsoft#24045) [catch2] Don't install docs (microsoft#24046) [harfbuff] fix arm64 osx build (microsoft#24055) [openxr-loader] remove from CI baseline (microsoft#24057) [imath] Update to 3.1.5 (microsoft#24059) [openssl] Fix dynamic builds on UNIX (microsoft#24061) [c-ares] update to 1.18.1 (microsoft#24062) [igraph] update to 0.9.8 (microsoft#24065) [cmake-user] Fix library check (microsoft#24070) [openxr-loader] fix ci.baseline.txt (microsoft#24073) [tinycbor] Fix file conflicts with libcbor (microsoft#24056) [graphviz,libslirp] Limit msys to windows (microsoft#24032) [bdwgc] Don't build docs (microsoft#24025) [capstone] update to 5.0.0-rc2 (microsoft#23979) [clockutils] Fix x64-windows-static-md (microsoft#23965) [braft] New port (microsoft#23830) ...

strega-nil-ms · 2022-04-13T17:55:06Z

@walbourn for that, I'd personally say something like xaudio::xaudio. In order to link fmt, for example, you'd link fmt::fmt.

Using Microsoft:: as a namespace is fine, it's just not standard practice in CMake land.

walbourn added 2 commits April 9, 2022 15:41

[dstorage] new port for Microsoft.Direct3D.DirectStorage NuGet

e73ee5b

Format fix

fc742b9

walbourn changed the title ~~[dstorage] - Add port for Microsoft.Direct3D.DirectStorage NuGet~~ [dstorage] Add port for Microsoft.Direct3D.DirectStorage NuGet Apr 9, 2022

github-actions bot requested changes Apr 9, 2022

View reviewed changes

Update baseline

d19b8b2

github-actions bot approved these changes Apr 9, 2022

View reviewed changes

Osyotr reviewed Apr 10, 2022

View reviewed changes

ports/dstorage/vcpkg.json Show resolved Hide resolved

JonLiu1993 assigned LilyWangLL Apr 11, 2022

LilyWangLL added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Apr 11, 2022

LilyWangLL approved these changes Apr 11, 2022

View reviewed changes

LilyWangLL added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Apr 11, 2022

strega-nil-ms suggested changes Apr 11, 2022

View reviewed changes

ports/dstorage/Config.cmake.in Outdated Show resolved Hide resolved

ports/dstorage/Config.cmake.in Show resolved Hide resolved

strega-nil-ms added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Apr 11, 2022

walbourn added 2 commits April 11, 2022 12:18

Moved headers up a folder per code review

af7c466

Update baselnie

fd7043a

github-actions bot approved these changes Apr 11, 2022

View reviewed changes

walbourn requested a review from strega-nil-ms April 11, 2022 19:28

walbourn added 2 commits April 11, 2022 16:07

More code review feedback

5110137

Refresh baseline

49fe84d

github-actions bot approved these changes Apr 11, 2022

View reviewed changes

walbourn mentioned this pull request Apr 11, 2022

[dxsdk-d3dx, xaudio2redist] ports updated to use config find #24054

Merged

strega-nil reviewed Apr 11, 2022

View reviewed changes

ports/dstorage/portfile.cmake Show resolved Hide resolved

strega-nil-ms merged commit 4415a0f into microsoft:master Apr 12, 2022

walbourn deleted the dstorage branch April 12, 2022 00:28

Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Apr 12, 2022

Merge pull request #984 from microsoft/master

4c04b91

[dstorage] Add port for Microsoft.Direct3D.DirectStorage NuGet (microsoft#24063)

walbourn mentioned this pull request Apr 12, 2022

[dstorage, dxsdk-d3dx, xaudio2redist] ports updated to support static-md triplets #24104

Merged

Conversation

walbourn commented Apr 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

walbourn commented Apr 9, 2022

Uh oh!

Uh oh!

strega-nil-ms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coopp commented Apr 11, 2022

Uh oh!

strega-nil commented Apr 11, 2022

Uh oh!

strega-nil commented Apr 11, 2022

Uh oh!

walbourn commented Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

walbourn commented Apr 11, 2022

Uh oh!

strega-nil commented Apr 11, 2022

Uh oh!

Uh oh!

walbourn commented Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strega-nil commented Apr 11, 2022

Uh oh!

walbourn commented Apr 11, 2022

Uh oh!

strega-nil commented Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

walbourn commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strega-nil commented Apr 12, 2022

Uh oh!

walbourn commented Apr 12, 2022

Uh oh!

strega-nil-ms commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

walbourn commented Apr 9, 2022 •

edited

Loading

walbourn commented Apr 11, 2022 •

edited

Loading

walbourn commented Apr 11, 2022 •

edited

Loading

strega-nil commented Apr 11, 2022 •

edited

Loading

walbourn commented Apr 12, 2022 •

edited

Loading