package HLSL2GLSLConverterImpl.hpp
AndreyMlashkin opened this issue · 32 comments
This file is not marked as a public interface and therefore not installed:
Graphics/HLSL2GLSLConverterLib/include/HLSL2GLSLConverterImpl.hpp
Nevertheless, diligent-tools includes this file in HLSL2GLSLConverter/src/HLSL2GLSLConverterApp.cpp
Could you please also add HLSL2GLSLConverterImpl.hpp to install targets or even better to separate HLSL2GLSLConverter to a new repository
Thanks a log for a very quick fix!
It would be really great it you also tag current core and tools versions, I could then release a new conan version of diligent.
@AndreyMlashkin I wanted to discuss how it can be added without any patches:
- 0019-252003: I can think how to add an option to disable all tests
- 0020-252003: Is the main problem here that License files don't exist? If so, I can add checks
- 0021-252003 Can you in the master CMake add the
glslangtarget that will link all required dependencies? Also, define alias forglslangasglslang::glslang? - 0022-252003 - need to think how to fix this
- 0023-252003 - The
-Werroroption can now be overriden by changing the DILIGENT_CLANG_COMPILE_OPTIONS. - 0024-252003: same as
0020-252003- will checks if the licenses exist work? - 0025-252003 - same as 23 - override
DILIGENT_MSVC_COMPILE_OPTIONS
So it looks like except for 19 and 22, all other patches can be avoided, WDYT?
19: option to disable all tests would spare this patch, yes
20: yes, cmake fails, if no file exists. Conan does not checkout submodules, so there is no file there. A check, if file exists would help. I am not sure, if there is a possibility to ignore an error, if no file is there in install() command
21: Hmmm, yes, that would spare a patch and I will try it
22: try to remove glslang in this line in the main CMakeLists.txt:
"set_directory_root_folder("glslang" "DiligentCore/ThirdParty/glslang")"
23: thanks for the hint, I will try
24: yes
25: I will try. I don't yet know, if /W4 ... /W0 is a correct flags combination after all
Actually, for the licenses patches:
- 20: These files are checked into the source tree, they are not in submodules. So they should be installed properly
- 24: The licenses should only be installed if targets are not defined outside of Diligent.
Can you try to see if it works without these patches already? It seems to me that they are no longer needed.
I don't yet know, if /W4 ... /W0 is a correct flags combination after all
You can remove /WX from DILIGENT_MSVC_COMPILE_OPTIONS. You will then get warnings, but they will not cause build failure.
Why do you get warnings BTW? They should all be fixed.
It fails here:
/home/andrei/.conan/data/diligent-core/2.5.2/_/_/build/6881f80c17218d135fdf22c03d23b300c1292b03/source_subfolder/Graphics/ShaderTools/src/GLSLangUtils.cpp:36:13: warning: 'ENABLE_HLSL' macro redefined [-Wmacro-redefined]
# define ENABLE_HLSL
^
<command line>:7:9: note: previous definition is here
#define ENABLE_HLSL 1
^
1 warning generated.
It fails here:
This happens when?
Also, do you define ENABLE_HLSL in the command line? There is a single line in the whole project where it is defined otherwise (where the error points).
It's not defined in a recipe. But grepping through the project shows, that is it defined in cmake:
ThirdParty/CMakeLists.txt: set(SPIRV_CROSS_ENABLE_HLSL OFF CACHE BOOL "Enable HLSL target support.")
Graphics/ShaderTools/CMakeLists.txt:set(ENABLE_HLSL FALSE)
Graphics/ShaderTools/CMakeLists.txt: set(ENABLE_HLSL TRUE)
Graphics/ShaderTools/CMakeLists.txt:if(ENABLE_HLSL)
Graphics/ShaderTools/src/GLSLangUtils.cpp:# define ENABLE_HLSL
If I remove install patch, it fails here:
Install the project...
-- Install configuration: "Debug"
-- Installing: /home/andrei/.conan/data/diligent-core/api.252003/andrei/test/package/6881f80c17218d135fdf22c03d23b300c1292b03/lib/source_subfolder/Debug/libDiligentCore.a
-- Installing: /home/andrei/.conan/data/diligent-core/api.252003/andrei/test/package/6881f80c17218d135fdf22c03d23b300c1292b03/Licenses/DiligentEngine-License.txt
-- Installing: /home/andrei/.conan/data/diligent-core/api.252003/andrei/test/package/6881f80c17218d135fdf22c03d23b300c1292b03/lib/source_subfolder/Debug/libglew-static.a
-- Installing: /home/andrei/.conan/data/diligent-core/api.252003/andrei/test/package/6881f80c17218d135fdf22c03d23b300c1292b03/Licenses/ThirdParty/source_subfolder/GLEW-License.txt
CMake Error at source_subfolder/ThirdParty/cmake_install.cmake:54 (file):
file INSTALL cannot find
"/home/andrei/.conan/data/diligent-core/api.252003/andrei/test/build/6881f80c17218d135fdf22c03d23b300c1292b03/source_subfolder/ThirdParty/SPIRV-Headers/LICENSE":
No such file or directory.
Call Stack (most recent call first):
source_subfolder/cmake_install.cmake:55 (include)
cmake_install.cmake:47 (include)
In CMake it defines the CMake variable, not the c++ define. I don't understand where the original definition comes from in your case.
How is then cmake variable forwarded to C++ code?
It is not forwarded. It is named the same, but it only defines the logic of CMake
0021 relates to conan-io/conan-center-index#11702
I have also added an override for compilation options in here
conan-io/conan-center-index@9cb21aa
If I remove install patch, it fails here:
Try to define backwards alias in the master CMake:
add_library(SPIRV-Headers ALIAS SPIRV::Headers)
If this does not work, can you comment out license installation commands one-by-one to see which ones exactly fail?
I have also added an override for compilation options in here
Like I said, there is no need to override /W4 with /W0. You need to remove /WX, which can be done by setting DILIGENT_MSVC_COMPILE_OPTIONS to empty string like you do with DILIGENT_CLANG_COMPILE_OPTIONS
If I remove install patch, it fails here:
Try to define backwards alias in the master CMake:
add_library(SPIRV-Headers ALIAS SPIRV::Headers)If this does not work, can you comment out license installation commands one-by-one to see which ones exactly fail?
Yes, that worked, I have removed two more patches
Could you please produce a new tag? I will create a new conan packages. For me it is important to add HLSL2GLSLConverterImpl to diligent-tools. Currently, it's not built with conan
Yes, that worked, I have removed two more patches
👍
As to 22: glslang::glslang target imported from the package and glslang defined by CMake directly use different public include directories, which is incorrect. I would suggest fixing it by defining the interface glslang library instead of ALIAS in the master CMake:
add_library(glslang INTERFACE)
target_link_libraries(glslang INTERFACE glslang::glslang)
target_include_directories(glslang INTERFACE path/to/glslang/glslang)Could you please produce a new tag?
Yes, let's figure out the remaining issues with patches. I think we are left with 22 and 19. And also I still don't understand why ENABLE_HLSL gets redefined.
I have found, where is this definition comes from:
self.cpp_info.components["glslang-core"].defines.append("ENABLE_HLSL")
it adds a define to C++ code "from console"
So, it is maximum I can do with the latest diligent-core tag.
I have tried to remove a warning by setting
self._cmake.definitions["DILIGENT_CLANG_COMPILE_OPTIONS"] = "-Wno-builtin-macro-redefined"
But it adds options and it does not override -Werror:
# define ENABLE_HLSL
^
<command line>:7:9: note: previous definition is here
#define ENABLE_HLSL 1
^
1 error generated.
applied options: [-Werror,-Wmacro-redefined]
So, my version from PR contains only two patches:
0019-252003-exclude-tests.patch
0023-252003-fix-warning-as-error.patch
As I see, there should be some work to be done in diligent:
- adding a flag, disabling all tests
- Werror should be a default value of DILIGENT_CLANG_COMPILE_OPTIONS
I have found, where is this definition comes from:
OK, so there is a global definition, which explains the issue. 43ae908 should fix it.
So, it is maximum I can do with the latest diligent-core tag.
The new DILIGENT_CLANG_COMPILE_OPTIONS was added after the latest tag. With the ENABLE_HLSL fix you may not need to override the Werror.
adding a flag, disabling all tests
Yes, I will take a look.
Werror should be a default value of DILIGENT_CLANG_COMPILE_OPTIONS
It already is. But if you are not using the latest version, it is not available.
As to 0019, what error do you get if you add Tests directory? If there is no gtest target, it should do nothing
43ae908 is merged - you can test if it will work with -Werror / /WX
I have tried it locally, it works. You can apply this patch and check on your own:
diff --git a/recipes/diligent-core/all/conandata.yml b/recipes/diligent-core/all/conandata.yml
index 1c398ce59..990f0da52 100644
--- a/recipes/diligent-core/all/conandata.yml
+++ b/recipes/diligent-core/all/conandata.yml
@@ -1,4 +1,7 @@
sources:
+ "cci.20220718":
+ url: "https://github.com/DiligentGraphics/DiligentCore/archive/43ae908b68c3fe140e0caebd02b7bf04311c211e.zip"
+ sha256: "f69d09f5ec0e7cf95b8cc0224552df0badbf45b873a9dcbd7f4fa4f9ff41034c"
Could you please create a tag then? From now on, it should be very easy to do
I have tried it locally, it works. You can apply this patch and check on your own:
What exactly works?
Producing a new conan version from current diligent-core master
So no patches needed now?
For current master no patches needed. Once you create a new tag, I will start a PR, creating a new conan version :)
👍
Will add a new tag to this commit.
Added tag API252004