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

Port for Microsoft GitHub DirectX-Headers #15222

Merged
merged 7 commits into from
Dec 27, 2020
Merged

Port for Microsoft GitHub DirectX-Headers #15222

merged 7 commits into from
Dec 27, 2020

Conversation

walbourn
Copy link
Member

There is a new open source GitHub project for the latest official DirectX 12 headers. This PR adds a port for it.

@NancyLi1013 NancyLi1013 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Dec 21, 2020
ports/directx-headers/CONTROL Show resolved Hide resolved
ports/directx-headers/portfile.cmake Outdated Show resolved Hide resolved
scripts/ci.baseline.txt Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

The failures on osx:

/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/src/dxguids.cpp:8:
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/wsl/winadapter.h:301:13: error: unknown type name 'constexpr'
template <> constexpr GUID uuidof<IUnknown>()
            ^
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/wsl/winadapter.h:301:1: error: extraneous 'template<>' in declaration of variable 'GUID'
template <> constexpr GUID uuidof<IUnknown>()
^~~~~~~~~~~
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/wsl/winadapter.h:301:27: error: expected ';' at end of declaration
template <> constexpr GUID uuidof<IUnknown>()
                          ^
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/wsl/winadapter.h:301:28: error: C++ requires a type specifier for all declarations
template <> constexpr GUID uuidof<IUnknown>()
                           ^
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/wsl/winadapter.h:301:28: error: template specialization requires 'template<>'
template <> constexpr GUID uuidof<IUnknown>()
                           ^     ~~~~~~~~~~
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/wsl/winadapter.h:303:12: error: excess elements in scalar initializer
    return { 0x00000000, 0x0000, 0x0000, { 0xC0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x46 } };
           ^           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/src/dxguids.cpp:12:
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/directx/d3d12.h:1284:1: error: unknown type name 'constexpr'
DEFINE_ENUM_FLAG_OPERATORS( D3D12_COMMAND_QUEUE_FLAGS );
^
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/wsl/winadapter.h:272:8: note: expanded from macro 'DEFINE_ENUM_FLAG_OPERATORS'
inline constexpr ENUMTYPE operator | (ENUMTYPE a, ENUMTYPE b) { return ENUMTYPE(((_ENUM_FLAG_SIZED_INTEGER<ENUMTYPE>::type)a) | ((_ENUM_FLAG_SIZED_INTEGER<ENUMTYPE>::type)b)); } \
       ^
In file included from /Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/src/dxguids.cpp:12:
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/directx/d3d12.h:1284:1: error: expected ';' after top level declarator
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/wsl/winadapter.h:272:27: note: expanded from macro 'DEFINE_ENUM_FLAG_OPERATORS'
inline constexpr ENUMTYPE operator | (ENUMTYPE a, ENUMTYPE b) { return ENUMTYPE(((_ENUM_FLAG_SIZED_INTEGER<ENUMTYPE>::type)a) | ((_ENUM_FLAG_SIZED_INTEGER<ENUMTYPE>::type)b)); } \
                          ^
In file included from /Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/src/dxguids.cpp:12:
/Users/vagrant/Data/buildtrees/directx-headers/src/v1.0-bee0ca9b39.clean/include/directx/d3d12.h:1973:1: error: unknown type name 'constexpr'
DEFINE_ENUM_FLAG_OPERATORS( D3D12_PIPELINE_STATE_FLAGS );
^

@walbourn
Copy link
Member Author

The failures on osx: ...

Supporting Linux, Windows, and UWP is a requirement, but not OSX.

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Dec 23, 2020
@NancyLi1013
Copy link
Contributor

LGTM now, thanks for your PR @walbourn.

@BillyONeal
Copy link
Member

Do these headers conflict with the ones that come with the Windows SDK in use and/or is it likely to invalidate caching?

@walbourn
Copy link
Member Author

@BillyONeal These are explicitly 'newer' than the Windows SDK releases by design, so they should always come first.

@walbourn
Copy link
Member Author

Supporting Linux, Windows, and UWP is a requirement, but not OSX.

BTW, I believe the issue on OSX is that the upstream CMakeLists.txt doesn't mention that it requires C++11 or C++14. Whenever they update it, I can try that as a tweak.

@BillyONeal
Copy link
Member

@BillyONeal These are explicitly 'newer' than the Windows SDK releases by design, so they should always come first.

Right, but that means that, for example

vcpkg install directx-headers
vcpkg install qt5-base

gives you different output than

vcpkg install qt5-base
vcpkg install directx-headers

i.e. the existence of this port can break binary caching of other ports.

I'm not sure what the right fix is though. I'll ask around.

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Dec 23, 2020
@walbourn
Copy link
Member Author

walbourn commented Dec 23, 2020

I'm not sure what the right fix is though. I'll ask around.

qt5-base should probably update to depend on directx-headers...

@BillyONeal
Copy link
Member

The problem is that because these headers come with the Windows PSDK, we don't know which ports would have to get such updates.

@walbourn
Copy link
Member Author

walbourn commented Dec 23, 2020

@BillyONeal Any open source package that includes Windows PSDK headers today is probably in violation of the Windows SDK license agreement in the first place. To my knowledge, the directxmath and directx-headers repos are some of the only 'blessed with MIT license' versions of Windows SDK headers.

In any case, given the target developers for these I doubt anyone using QT, for example, is going to use directxmath and/or directx-headers in the first place.

That said, if a client library is using the 'core' d3d12.h or dxgiformat.h stuff that's in the Windows SDK version, then that's going to be there in directx-headers. The POR is that directx-headers is going to be strictly newer than any existing Windows 10 SDK versions. Since these are all nano-COM and use known ABI, it shouldn't pose any problem if you accidentally end up with an 'old' header and a 'new' import library. The only problem will be a compile-time failure if you need header content from directx-headers that isn't in your Windows 10 SDK in which case you have to fix the usage order in your CMakeLists/vcproj.

@BillyONeal
Copy link
Member

@BillyONeal Any open source package that includes Windows PSDK headers today is probably in violation of the Windows SDK license agreement in the first place.

This isn't about open source packages that contain Windows headers. This is about open source packages that consume PSDK headers. Before this port exists, all those headers come from the system (morally, the triplet), since they're part of the Windows SDK that comes with your compiler. After this port exists, where the headers come from depends on whether this port is installed, which generally shouldn't happen. That is, if vcpkg install port works, then vcpkg install anotherPort port should work, but that's not necessarily true here.

I used qt as my example because we currently have an issue where nightly CI for x64-windows-static randomly fails depending on whether directxsdk is installed before or after the qt bits. If the qt bits go first, the directxsdk parts come from the Windows SDK, which is what qt's authors intend, and succeeds. But if the directxsdk port goes first the libs get selected from that port, which breaks qt's build. This PR expands that problem to include headers too.

We have post-build checks when installing multiple ports to make sure only one port provides a given entity in the installed tree but we haven't really addressed the "installed tree conflicts with the PSDK" situation before.

In any case, given the target developers for these I doubt anyone using QT, for example, is going to use directxmath and/or directx-headers in the first place.

It's possible that the right fix is indeed to accept these ports but skip them in ci.baseline.txt (meaning the vcpkg infrastructure will never try to validate that they succeed). But those cases are usually reserved for problems that vcpkg itself detects, like trying to install boringssl and openssl at the same time (which we detect as a failure because 2 different ports try to provide the same headers).

Since these are all nano-COM and use known ABI, it shouldn't pose any problem if you accidentally end up with an 'old' header and a 'new' import library.

It poses a problem in that it breaks binary caching, since it affects the compiled output for all other ports in the system in a way that doesn't affect their hash.

@walbourn
Copy link
Member Author

walbourn commented Dec 23, 2020

@BillyONeal Actually, I forgot to mention an important detail here that was discussed in-depth with the DirectX-Headers maintainers a while back...

With or without directx-headers, the following client code will always use the Windows 10 PSDK headers:

#include <d3d12.h>
#include <dxgiformat.h>

And the following will use the existing "d3dx12.h" in the project--it's not part of the Windows 10 PSDK:

#include <d3dx12.h>

In order to consume the headers in the directx-headers package, client code must be modified:

#include <directx/d3d12.h>
#include <directx/dxgiformat.h>
#include <directx/d3dx12.h>

The dxguids/dxguids.h header and DirectX-Guids.lib are new and are not ambiguous with the Windows 10 PSDK.

@BillyONeal
Copy link
Member

@BillyONeal Actually, I forgot to mention an important detail here that was discussed in-depth with the DirectX-Headers maintainers a while back...

[...]

That indeed resolves this exact concern, thank you, and thanks for the patience :)

@BillyONeal BillyONeal merged commit 4a678aa into microsoft:master Dec 27, 2020
@walbourn walbourn deleted the user/chuckw/directxheaders branch December 28, 2020 23:21
ryukw7 pushed a commit to ryukw7/vcpkg that referenced this pull request Dec 29, 2020
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.

3 participants