Skip to content

Conversation

@LazyJazz
Copy link

@LazyJazz LazyJazz commented Mar 27, 2025

Fixes #44644

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

@LazyJazz LazyJazz changed the title Dxc osx [directx-dxc] Make compatible with macOS Mar 27, 2025
@LazyJazz
Copy link
Author

@microsoft-github-policy-service agree

@LazyJazz
Copy link
Author

LazyJazz commented Mar 27, 2025

The idea is to pull the source code from DirectXShaderCompiler, compile with local environment then copy the target binary files into packages directory.

Possible issue(s):

  • Since the module repository contains submodules, I couldn't find a way to fetch submodules with the main repo in vcpkg_from_github function, so I introduced 3 individual git clone and git checkout respectively. This is not a elegant implementation.
  • I could not find the appropriate variable for target directories, just find ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel and ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-dbg works right.
  • Compiling the library on user device is too time consuming. Could you ask the group which's maintaining DirectXShaderCompiler repo provide an official release for macOS?

Hope there's better solutions for above issues.

@WangWeiLin-MV WangWeiLin-MV self-assigned this Mar 28, 2025
@WangWeiLin-MV WangWeiLin-MV added category:port-bug The issue is with a library, which is something the port should already support category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist and removed category:port-bug The issue is with a library, which is something the port should already support labels Mar 28, 2025
@LazyJazz LazyJazz marked this pull request as ready for review March 28, 2025 09:42
@LazyJazz
Copy link
Author

@WangWeiLin-MV May I have some feedback?

WangWeiLin-MV
WangWeiLin-MV previously approved these changes Mar 31, 2025
@WangWeiLin-MV WangWeiLin-MV added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Mar 31, 2025
@LazyJazz LazyJazz marked this pull request as draft April 2, 2025 01:48
@LazyJazz LazyJazz marked this pull request as ready for review April 2, 2025 09:02
@LazyJazz LazyJazz requested a review from WangWeiLin-MV April 4, 2025 00:08
@vicroms
Copy link
Member

vicroms commented Apr 4, 2025

@walbourn are these changes OK to you?

@dg0yt
Copy link
Contributor

dg0yt commented Apr 4, 2025

  • It looks like macOS could be aligned a litte bit more along linux.
  • The patch seems to need cleanup.
  • There are ports for the extra directx stuff IIUC.

@walbourn
Copy link
Member

walbourn commented Apr 4, 2025

@walbourn are these changes OK to you?

I don't know how sustainable macOS support is here unless it can be made to work like Linux. The use of submodules rather than other dependent ports feels highly problematic to me.

@LazyJazz
Copy link
Author

LazyJazz commented Apr 5, 2025

@walbourn are these changes OK to you?

I don't know how sustainable macOS support is here unless it can be made to work like Linux. The use of submodules rather than other dependent ports feels highly problematic to me.

The reason of not using dx-headers port is that the latest dx-headers lib doesn't work with dxc. Even in the latest commit of original DXC repo, the referenced dx-headers is still using the version from 2022. It's not my responsibility to fix such compatibility issue, I'm here only to offer a possible solution for the requirement of enabling dxc library on macOS. If there's a better solution, that would be providing an official build for macOS platform. Thank you.

@LazyJazz
Copy link
Author

LazyJazz commented Apr 5, 2025

@walbourn are these changes OK to you?

I don't know how sustainable macOS support is here unless it can be made to work like Linux. The use of submodules rather than other dependent ports feels highly problematic to me.

By the way, the directx-headers port is not supporting osx either. Using submodule under this specific circumstance won't cause conflict between versions of the same library.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 5, 2025

By the way, the directx-headers port is not supporting osx either. Using submodule under this specific circumstance won't cause conflict between versions of the same library.

This underlines the problems with the current appproach:
When directx-headers is changed to declare osx support, one wouldn't notice that it could interfere with this port's vendored copy.
And vcpkg install directx-headers:x64-osx --allow-unsupported works immediately...

@LazyJazz
Copy link
Author

LazyJazz commented Apr 5, 2025

This underlines the problems with the current appproach: When directx-headers is changed to declare osx support, one wouldn't notice that it could interfere with this port's vendored copy. And vcpkg install directx-headers:x64-osx --allow-unsupported works immediately...

Thank you for the comment. Then I'm curious why the compiled dxc binaries for Windows and Linux could be imported securely? IIUC, those binary also used 'vendored copy' of directx headers because they are compiled from the same repo which is referring to this 2022 directx headers lib. If what you said could be a problem, so does on these existing contents.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 5, 2025

Indeed, the port avoids conflicts by not installing ("directx") headers.

@walbourn The problems with building from source using new directx headers start with a lot of duplicate definitions in the Windows polyfill (basetsd.h). This could probably be observed also on Linux. In addition, Apple's clang has complaints about some typedefs' validity.

@LazyJazz
Copy link
Author

LazyJazz commented Apr 5, 2025

Indeed, the port avoids conflicts by not installing ("directx") headers.

@walbourn The problems with building from source using new directx headers start with a lot of duplicate definitions in the Windows polyfill (basetsd.h). This could probably be observed also on Linux. In addition, Apple's clang has complaints about some typedefs' validity.

Well, this PR I'm submitting also avoided installation of DirectX headers. Those involved outdated dx headers module are only used during compilation stage in the package installation process. The installed files are basically the same as how the Linux's are. No dx headers would be installed to the package folder. If the solution for Linux is acceptable, I don't see any difference between this solution for macOS and the solution for Linux except for whether the binary files is compiled as an official release or by user's local device.

The following is the installed file list in this solution:

- include/
  - directx-dxc/
    - dxcapi.h
    - dxcerrors.h
    - dxcisense.h
    - WinAdapter.h
- lib/
  - libdxcompiler.dylib
  - libdxil.dylib
- debug/
  - lib/
    - libdxcompiler.dylib
    - libdxil.dylib

and these are the installed file in Linux solution:

- include/
  - directx-dxc/
    - dxcapi.h
    - dxcerrors.h
    - dxcisense.h
    - WinAdapter.h
- lib/
  - libdxcompiler.so
  - libdxil.so
- debug/
  - lib/
    - libdxcompiler.so
    - libdxil.so

What could be wrong if the Linux's is acceptable while this macOS's isn't?

@dg0yt
Copy link
Contributor

dg0yt commented Apr 5, 2025

Indeed, the port avoids conflicts by not installing ("directx") headers.

This was meant to capture what you presented in more detail in the last post. i.e. there is nothing more wrong with the resulting osx installation than with the linux installation.

Except that the long-term perspective of osx support is officially unclear at the moment.
And that the definition inconsistencies could still make it into user projects, even for x64-linux. This is a drive-by discovery, not introduced by your PR.

directx-headers installs include/wsl/winadapter.h.
directx-dxc installs include/directx-dxc/WinAdapter.h.
For osx (or host windows) with a case-insensitive file system, this is the same filename.

include/wsl/winadapter.h has basetsd.h has

typedef uint32_t UINT32, UINT, ULONG, DWORD, BOOL, WINBOOL;

include/wsl/WinAdapter.h has

typedef bool BOOL;
...
typedef unsigned int UINT;
typedef unsigned long ULONG;
typedef unsigned long ULONG;

@LazyJazz
Copy link
Author

LazyJazz commented Apr 5, 2025

Indeed, the port avoids conflicts by not installing ("directx") headers.

This was meant to capture what you presented in more detail in the last post. i.e. there is nothing more wrong with the resulting osx installation than with the linux installation.

Except that the long-term perspective of osx support is officially unclear at the moment. And that the definition inconsistencies could still make it into user projects, even for x64-linux. This is a drive-by discovery, not introduced by your PR.

directx-headers installs include/wsl/winadapter.h. directx-dxc installs include/directx-dxc/WinAdapter.h. For osx (or host windows) with a case-insensitive file system, this is the same filename.

include/wsl/winadapter.h has basetsd.h has

typedef uint32_t UINT32, UINT, ULONG, DWORD, BOOL, WINBOOL;

include/wsl/WinAdapter.h has

typedef bool BOOL;
...
typedef unsigned int UINT;
typedef unsigned long ULONG;
typedef unsigned long ULONG;

Got it, thanks. Then should I hang up this update until official maintainers fix-up such conflict?


# for macOS platform:
# The DirectX Headers module seems not working with the latest version
# Therefore we manually download the specific version according to the original DXC repo
Copy link
Member

Choose a reason for hiding this comment

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

Have you submitted fixes for the DirectXHeaders repo?

Copy link
Author

Choose a reason for hiding this comment

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

No, I don't think it's a simple issue to fix and I currently don't have such ability to fix it. I found other's PR to fix such issue but seems like it was not replied. microsoft/DirectXShaderCompiler: Update directx headers to facilitate future updates #6856

@walbourn
Copy link
Member

walbourn commented Apr 8, 2025

I appreciate the improvements to use VCPKG packages instead of direct git clones.

Can you clarify what the scenario is here for having this code run on macOS? DirectX 12 is supported on Windows and on Windows Subsystem for Linux, but it doesn't work at all on macOS.

@LazyJazz
Copy link
Author

LazyJazz commented Apr 8, 2025

I appreciate the improvements to use VCPKG packages instead of direct git clones.

Can you clarify what the scenario is here for having this code run on macOS? DirectX 12 is supported on Windows and on Windows Subsystem for Linux, but it doesn't work at all on macOS.

Well, as dxc now support compiling HLSL to SPIRV, it could be used with Vulkan on macOS. In my personal scenario, I'm writing one piece of shader code that could be applied to both DX12 and Vulkan on all platforms.

@PhoebeHui PhoebeHui assigned LilyWangLL and unassigned WangWeiLin-MV Apr 9, 2025
@BillyONeal BillyONeal requested a review from walbourn April 10, 2025 16:53
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

This approval is contingent on @walbourn also approving

@BillyONeal BillyONeal closed this Apr 17, 2025
@BillyONeal BillyONeal reopened this Apr 17, 2025
@LazyJazz
Copy link
Author

I realized that Vulkan SDK installation has a copy of dxc binaries within. It's not necessary to import an external dxc library.

@LazyJazz LazyJazz closed this Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[directx-dxc] Require DXC compatibility on macOS platform

7 participants