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

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

Merged
merged 7 commits into from
Apr 12, 2022

Conversation

walbourn
Copy link
Member

@walbourn walbourn commented Apr 9, 2022

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.

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

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

walbourn commented Apr 9, 2022

Validated all supported triplets:

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

@LilyWangLL 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 LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Apr 11, 2022
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
ports/dstorage/Config.cmake.in Outdated Show resolved Hide resolved
@strega-nil-ms strega-nil-ms added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Apr 11, 2022
@walbourn walbourn requested a review from strega-nil-ms April 11, 2022 19:28
@coopp
Copy link

coopp commented Apr 11, 2022

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

@strega-nil
Copy link
Contributor

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

@strega-nil
Copy link
Contributor

I also think the following changes should be made:

From 6dbded87ea8f0eb38b9bfda2605f26998abd7726 Mon Sep 17 00:00:00 2001
From: nicole mazzuca <[email protected]>
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
Copy link
Member Author

walbourn commented Apr 11, 2022

@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
Copy link
Member Author

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
Copy link
Contributor

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

@walbourn
Copy link
Member Author

walbourn commented Apr 11, 2022

@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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

strega-nil commented Apr 11, 2022

@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
Copy link
Member Author

walbourn commented Apr 12, 2022

@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
Copy link
Contributor

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

@strega-nil-ms strega-nil-ms merged commit 4415a0f into microsoft:master Apr 12, 2022
@walbourn walbourn deleted the dstorage branch April 12, 2022 00:28
@walbourn
Copy link
Member Author

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.

Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Apr 12, 2022
[dstorage] Add port for Microsoft.Direct3D.DirectStorage NuGet (microsoft#24063)
ekilmer added a commit to ekilmer/vcpkg that referenced this pull request Apr 12, 2022
* 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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants