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

[directxsdk] Update DirectX SDK port #16010

Merged
merged 8 commits into from
Feb 9, 2021
Merged

[directxsdk] Update DirectX SDK port #16010

merged 8 commits into from
Feb 9, 2021

Conversation

walbourn
Copy link
Member

@walbourn walbourn commented Feb 3, 2021

The legacy DirectX SDK is no longer hosted on Microsoft Downloads due to the retirement of SHA-1 signed content.

This port does a number of things:

  • It uses the archive.org copy. That has been verified to be genuine, and the SHA-256 hash inside this port has not changed.
  • There's a warning from the port noting this is deprecated
  • Per the guidance in this blog post, I trimmed out the headers/libs known to conflict with the Windows SDK
  • Added optional features to add legacy xinput1-3 and xaudio2-7 which conflicts with the Windows 8 SDK / Windows 10 SDK headers -and- requires legacy setup. This supports opt-in use of the legacy bits.
  • Added optional feature xp to add all the content needed to build with the Windows 7.1A SDK

Note that the download speeds from archive.org are noticeably slower than they were on Microsoft Downloads.

@NancyLi1013 NancyLi1013 self-assigned this Feb 3, 2021
@NancyLi1013 NancyLi1013 changed the title Update DirectX SDK port [directxsdk] Update DirectX SDK port Feb 3, 2021
@NancyLi1013 NancyLi1013 added the category:port-update The issue is with a library, which is requesting update new revision label Feb 3, 2021
ports/directxsdk/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Feb 3, 2021
@NancyLi1013
Copy link
Contributor

LGTM now, thanks for your contribution @walbourn.

Copy link
Contributor

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

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

Great work!

ports/directxsdk/vcpkg.json Outdated Show resolved Hide resolved
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

This LGTM with @Hoikas' suggested change. Thanks a ton @walbourn for figuring out a way forward here :)

Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>
@NancyLi1013
Copy link
Contributor

Just need to run vcpkg x-add-version directxsdk and submit the changes to solve the versions check on x86-windows.

Found the following errors:
Error: While reading versions for port directxsdk from file: C:\a\1\s\versions\d-\directxsdk.json
       Version `jun10#1` was not found in versions file.
       Run:

           vcpkg x-add-version directxsdk

       to add the new port version.

@NancyLi1013 NancyLi1013 removed the info:reviewed Pull Request changes follow basic guidelines label Feb 4, 2021
@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Feb 4, 2021
@NancyLi1013
Copy link
Contributor

Ping @ras0219-msft for merge this PR.

@polelf
Copy link

polelf commented Feb 5, 2021

should make the following modifications, otherwise there will be link errors
For example

vcpkg install directxsdk 
vcpkg install qt5

patch:

 ports/directxsdk/portfile.cmake | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ports/directxsdk/portfile.cmake b/ports/directxsdk/portfile.cmake
index 86fa8a149..3b8a41722 100644
--- a/ports/directxsdk/portfile.cmake
+++ b/ports/directxsdk/portfile.cmake
@@ -52,8 +52,8 @@ endif()
 
 #install(DIRECTORY "${SOURCE_PATH}/Include" DESTINATION ${CURRENT_PACKAGES_DIR}/include)
 file(COPY "${SOURCE_PATH}/Include/" DESTINATION ${CURRENT_PACKAGES_DIR}/include/${PORT})
-file(COPY ${RELEASE_LIBS} ${OTHER_LIBS} DESTINATION ${CURRENT_PACKAGES_DIR}/lib)
-file(COPY ${DEBUG_LIBS} ${OTHER_LIBS} DESTINATION ${CURRENT_PACKAGES_DIR}/debug/lib)
+file(COPY ${RELEASE_LIBS} ${OTHER_LIBS} DESTINATION ${CURRENT_PACKAGES_DIR}/lib/${PORT})
+file(COPY ${DEBUG_LIBS} ${OTHER_LIBS} DESTINATION ${CURRENT_PACKAGES_DIR}/debug/lib/${PORT})
 

@NancyLi1013
Copy link
Contributor

@walbourn

Could you please address the above suggestion from @polelf ?

@NancyLi1013 NancyLi1013 added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Feb 5, 2021
@polelf
Copy link

polelf commented Feb 5, 2021 via email

@ras0219
Copy link
Contributor

ras0219 commented Feb 5, 2021

Hmm, it's not clear what to do here. While having the libraries directly in lib/ may cause issues with qt5, if they aren't in lib/ then they won't be linked by MSBuild users. If we do need to move them, they should go into lib/manual-link and not lib/${PORT}.

@walbourn You mentioned above that you've removed some things that conflict with the Win10 SDK; are you aware of any remaining conflicts?

@walbourn
Copy link
Member Author

walbourn commented Feb 5, 2021

@ras0219 I did not change the locations of any of the install locations in this port in this update. I only changed the content. I did not add any new libs, I only removed known conflicts and put a few others into feature choices. IOTW, whatever issue qt5 is having is not new to my PR here.

The problem is that qt5 shouldn't be including legacy DirectX SDK libs as that's not supported by the DirectX SDK EULA in the first place. Exactly which filenames are the problem here @polelf ?

@walbourn
Copy link
Member Author

walbourn commented Feb 6, 2021

@ras0219 It looks like this PR is still blocked "awaiting changes" but I don't have any pending to take... The change from @polelf would make the new port work differently than the old one in terms of file locations.

@walbourn
Copy link
Member Author

walbourn commented Feb 7, 2021

@NancyLi1013 , @ras0219 , @ras0219-msft, I've tried:

vcpkg install directxsdk:x86-windows
vcpkg install qt5:x86-windows
vcpkg install directxsdk:x64-windows
vcpkg install qt5:x64-windows

I've never gotten any link errors. I looked at the libraries installed by the qt5 port and none of them conflict with the filenames of the libraries published by the directxsdk port with any of the features. Unless @polelf has some additional details on the repro or link errors, I don't see any problem here to fix. There's no reason to move the lib location from the previous version of this port.

This PR is still blocked 'requested changes' by @ras0219-msft but there's nothing to change at this point.

@polelf
Copy link

polelf commented Feb 7, 2021

@walbourn you need clean vcpkg cache

qt5 does not use legacy directx sdk lib. First install directxsdk qt5 will link to dxguid under vcpkg/installed/lib, so there will be uuid missing

without modifying the directxsdk port lib install path, it cannot be installed at the same time with all ports that do not use legacy directxsdk

@ras0219-msft
Copy link
Contributor

I need to do some investigation about the interaction here; no current action required by @walbourn

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Feb 9, 2021

Ok, I think the right path forward is to disable directxsdk in our CI. It is a deprecated port that only exists for legacy reasons, using it is against the EULA so you can't actually ship anything based on it. It's therefore not worth spending hours on flaky conflicts between itself and the world.

It will still be available for existing customers and should generally work (thanks again @walbourn) after printing a nastygram.

@ras0219-msft ras0219-msft merged commit 23306bd into microsoft:master Feb 9, 2021
@walbourn walbourn deleted the directxsdkport branch February 11, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants