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

Removed OS condition to install DXCompiler #5713

Closed
wants to merge 4 commits into from
Closed

Removed OS condition to install DXCompiler #5713

wants to merge 4 commits into from

Conversation

bmarques1995
Copy link

added the archive(.lib) file to dxcompiler install

Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

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

I think the more correct fix to this bug is to change the add_clang_library macro defined in tools/clang/CMakeLists.txt to treat dxcompiler the same way it treats libclang.

@llvm-beanz
Copy link
Collaborator

I believe this is intended to fix #5682.

@bmarques1995
Copy link
Author

bmarques1995 commented Sep 14, 2023

I believe this is intended to fix #5682.

I was analysing the repo, and I'm only using the install rule originally added to the dxcompiler target, but, I've also added the archive destination, following the pattern of the target and default destination of cmake, this is only for this single install, not for the lib building.

@bmarques1995
Copy link
Author

I believe this is intended to fix #5682.

By the way, are you one of the analysers of this issue? I believe u've looked that I made it

@bmarques1995 bmarques1995 changed the title Added option to install archive file Removed OS condition to install DXCompiler Sep 14, 2023
@bmarques1995
Copy link
Author

I believe this is intended to fix #5682.

I've modified my PR, now I removed the OS condition to install target, this is unnecessary, cmake automaticly manages the binaries(runtime [executable and DLLs], library [static libs on Windows, or all libs on another distros] and archive[symbols{...dll.lib}]) of all OS, depending the target

@bmarques1995
Copy link
Author

@bmarques1995 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

Copy link
Collaborator

@llvm-beanz llvm-beanz 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 much more complicated than it needs to be. You should be able to add dxcompiler here:

https://github.com/microsoft/DirectXShaderCompiler/blob/main/tools/clang/CMakeLists.txt#L377

The stuff you're doing with the find_* is also not correct. I don't think we should make a DXC-specific CMake exports list because ultimately DXC is just a fork of LLVM. I think it would be better to connect these through the existing export mechanism for LLVM targets.

@bmarques1995
Copy link
Author

This is much more complicated than it needs to be. You should be able to add dxcompiler here:

https://github.com/microsoft/DirectXShaderCompiler/blob/main/tools/clang/CMakeLists.txt#L377

The stuff you're doing with the find_* is also not correct. I don't think we should make a DXC-specific CMake exports list because ultimately DXC is just a fork of LLVM. I think it would be better to connect these through the existing export mechanism for LLVM targets.

this is the only problem or don't you agree also with the new runtime syntax? if is this the case i discard the change of find_package export, but the first one for me is critical, to move the install syntax

@bmarques1995 bmarques1995 closed this by deleting the head repository Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants