add necessary windows lib files for vulkan, dx12, dx11 #192

Timo-DK · 2019-05-25T16:43:52Z

Introduce new argument BACKEND to specify the back-end framework in the hello_triangle_c CMake script. I will update the other examples, hello_remote_c & hello_compute_c (working on this one) in a future PR.

fix #188

introduce new argument feature-native to specify the back-end framework

cormac-obrien

Just one minor change, other than that everything LGTM.

Also, do we want this to be a full fix for #188? We still need Makefile changes to get everything to behave nicely.

cormac-obrien · 2019-05-25T18:37:01Z

examples/hello_triangle_c/CMakeLists.txt

 add_executable(hello_triangle main.c)

 if(MSVC)
    add_definitions(-DWGPU_TARGET=WGPU_TARGET_WINDOWS)
    target_compile_options(${TARGET_NAME} PRIVATE /W4)
    set(GLFW_LIBRARY glfw3)
+
+    target_link_libraries(${TARGET_NAME} userenv ws2_32 Dwmapi)


We also need to link dbghelp here for debug builds -- it looks like backtrace relies on it.

I don't know that about dbghelp, I could include it no problem, but do you experience problems without it?

Without it, I get a few backtrace-related linker errors:

wgpu_native.lib(backtrace-[...].o) : error LNK2019: unresolved external symbol SymInitializeW referenced in function _ZN9backtrace12dbghelp_init17h846b2405be6646eaE wgpu_native.lib(backtrace-[...].o) : error LNK2019: unresolved external symbol SymFromAddrW referenced in function _ZN9backtrace9symbolize7dbghelp7resolve17h87c86220145b776bE wgpu_native.lib(backtrace-[...].o) : error LNK2019: unresolved external symbol SymGetLineFromAddrW64 referenced in function _ZN9backtrace9symbolize7dbghelp7resolve17h87c86220145b776bE wgpu_native.lib(backtrace-[...].o) : error LNK2019: unresolved external symbol SymFunctionTableAccess64 referenced in function _ZN9backtrace9backtrace20trace_unsynchronized17he6ede2ba53fdf73fE wgpu_native.lib(backtrace-[...].o) : error LNK2019: unresolved external symbol SymGetModuleBase64 referenced in function _ZN9backtrace9backtrace20trace_unsynchronized17he6ede2ba53fdf73fE wgpu_native.lib(backtrace-[...].o) : error LNK2019: unresolved external symbol StackWalk64 referenced in function _ZN9backtrace9backtrace20trace_unsynchronized17he6ede2ba53fdf73fE C:\[...]\hello_triangle.exe : fatal error LNK1120: 6 unresolved externals

Oh I have not had those errors. I will look into it.

Timo-DK · 2019-05-25T19:49:34Z

Just one minor change, other than that everything LGTM.

Also, do we want this to be a full fix for #188? We still need Makefile changes to get everything to behave nicely.

Personally I like to push this first into the master branch, so that at least we can build a working version. I will look into the remaining issues after that, but I think that should be your call @kvark.

kvark · 2019-05-26T00:58:53Z

examples/hello_triangle_c/CMakeLists.txt

@@ -3,13 +3,20 @@ cmake_minimum_required(VERSION 3.11b)
 project(hello_triangle)

 set(TARGET_NAME hello_triangle)
-
+set(feature-native)


where do we set the value to "feature-native"?

Well that can actually be done by doing cmake .. -Dfeature-native=dx12, that was the only way I have found to do it now. If there is a better alternative, please tell me so and I will correct it.

Maybe we could name it BACKEND and require it (if no value was passed then fail). The purpose would be to match whichever BACKEND that libwgpu-native was built with originally (ideally this would be a list of backends, but we could do that in a future PR).

I agree, but for now it will be only used for dx12, so the require would not be necessary. I do not know if we have any information about what backend lib was built, so we would have to investigate that in what I propose a future issue.

I don’t think we need to try to derive it - in Travis CI we could just pass it directly, and we can document it for people running cmake manually.

I agree it’s not a big deal right now, but it’s better to be consistent and require it than special-case dx12 IMO

grovesNL · 2019-05-26T13:05:20Z

examples/hello_triangle_c/CMakeLists.txt

@@ -3,13 +3,20 @@ cmake_minimum_required(VERSION 3.11b)
 project(hello_triangle)

 set(TARGET_NAME hello_triangle)
-
+set(feature-native)


Maybe we could name it BACKEND and require it (if no value was passed then fail). The purpose would be to match whichever BACKEND that libwgpu-native was built with originally (ideally this would be a list of backends, but we could do that in a future PR).

grovesNL · 2019-05-26T13:06:07Z

examples/hello_triangle_c/CMakeLists.txt

@@ -27,4 +34,4 @@ find_library(WGPU_LIBRARY wgpu_native
    HINTS "${CMAKE_CURRENT_SOURCE_DIR}/../../target/debug"
 )

-target_link_libraries(${TARGET_NAME} ${GLFW_LIBRARY} ${WGPU_LIBRARY} ${OS_LIBRARIES})
+target_link_libraries(${TARGET_NAME} ${GLFW_LIBRARY} ${WGPU_LIBRARY} ${OS_LIBRARIES})


We should have a newline at the end

Habits I guess, I always clean the new lines up. Is there a reason for why this standard is chosen?

Some tools expect it to be there to function correctly, so it’s generally best practice to have a new line here. See https://stackoverflow.com/questions/5813311/no-newline-at-end-of-file/5813359 for some examples. GitHub also warns about this in the diff.

grovesNL · 2019-05-26T13:09:57Z

examples/hello_triangle_c/CMakeLists.txt

 add_executable(hello_triangle main.c)

 if(MSVC)
    add_definitions(-DWGPU_TARGET=WGPU_TARGET_WINDOWS)
    target_compile_options(${TARGET_NAME} PRIVATE /W4)
    set(GLFW_LIBRARY glfw3)
+
+    target_link_libraries(${TARGET_NAME} userenv ws2_32 Dwmapi)


We could just use OS_LIBRARIES to set these for MSVC

grovesNL · 2019-05-26T13:10:08Z

examples/hello_triangle_c/CMakeLists.txt

+    target_link_libraries(${TARGET_NAME} userenv ws2_32 Dwmapi)
+
+    if ("${feature-native}" STREQUAL "dx12")
+        target_link_libraries(${TARGET_NAME} d3dcompiler D3D12 DXGI)


I'd slightly prefer to have a variable for BACKEND_LIBRARIES and add it to the end of the existing target_link_libraries – like we do with OS_LIBRARIES

I do agree with your previous comment: to set the OS_LIBRARIES variable with these values. So the script will stay clean

- pass backends from the Makefile to the CMakeLists file - add check to see if the BACKEND argument is set - add dbghelp lib file

kvark · 2019-05-26T16:49:52Z

Just want to clarify that we support dx11 as well.

…

On May 26, 2019, at 11:59, Josh Groves ***@***.***> wrote: @grovesNL commented on this pull request. In examples/hello_triangle_c/CMakeLists.txt: > @@ -3,13 +3,20 @@ cmake_minimum_required(VERSION 3.11b) project(hello_triangle) set(TARGET_NAME hello_triangle) - +set(feature-native) I don’t think we need to try to derive it - in Travis CI we could just pass it directly, and we can document it for people running cmake manually. I agree it’s not a big deal right now, but it’s better to be consistent and require it than special-case dx12 IMO — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Timo-DK · 2019-05-26T17:19:07Z

Just want to clarify that we support dx11 as well.

I did not know that, after a quick search through the code, you are indeed right. The Makefile however, does not even specify the FEATURE_RUST=dx11 FEATURE_NATIVE=gfx-backend-dx11. Is this something that is planned to be supported?

kvark · 2019-05-26T17:37:33Z

Makefile backend selection is "best effort". With dx11 and recently added GL there is a need for more complex logic in there.

…

On May 26, 2019, at 13:19, Timo de Kort ***@***.***> wrote: Just want to clarify that we support dx11 as well. I did not know that, after a quick search through the code, you are indeed right. The Makefile however, does not even specify the FEATURE_RUST=dx11 FEATURE_NATIVE=gfx-backend-dx11. Is this something that is planned to be supported? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Timo-DK · 2019-05-26T17:47:38Z

Done, I have made it pretty flexible, so there is plenty of room for more platforms in the future, for instance GL soon.

kvark

Thank you!
bors r+

192: add necessary windows lib files for vulkan, dx12, dx11 r=kvark a=Napokue Introduce new argument BACKEND to specify the back-end framework in the hello_triangle_c CMake script. I will update the other examples, hello_remote_c & hello_compute_c (working on this one) in a future PR. fix #188 Co-authored-by: Timo de Kort <[email protected]>

bors · 2019-05-27T14:58:05Z

Build succeeded

continuous-integration/travis-ci/push

192: Explicitly declare vertex attribute descriptors in cube example r=kvark a=aloucks The new `vertex_attr_array!` macro is pretty wonderful, but it's nice to have at least one reference example of how `VertexAttributeDescriptor`s are constructed. Co-authored-by: Aaron Loucks <[email protected]>

add necessary windows lib files for vulkan/dx12

747defd

introduce new argument feature-native to specify the back-end framework

cormac-obrien suggested changes May 25, 2019

View reviewed changes

kvark reviewed May 26, 2019

View reviewed changes

grovesNL reviewed May 26, 2019

View reviewed changes

resolve PR comments

9b06079

- pass backends from the Makefile to the CMakeLists file - add check to see if the BACKEND argument is set - add dbghelp lib file

add DX11 support

7bb5281

Timo-DK changed the title ~~add necessary windows lib files for vulkan/dx12~~ add necessary windows lib files for vulkan, dx12, dx11 May 26, 2019

kvark approved these changes May 27, 2019

View reviewed changes

bors bot merged commit 7bb5281 into gfx-rs:master May 27, 2019

Timo-DK deleted the windows_cmake_fix branch May 27, 2019 20:18

add necessary windows lib files for vulkan, dx12, dx11 #192

add necessary windows lib files for vulkan, dx12, dx11 #192

Uh oh!

Conversation

Timo-DK commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cormac-obrien left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Timo-DK commented May 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark commented May 26, 2019 via email

Uh oh!

Timo-DK commented May 26, 2019

Uh oh!

kvark commented May 26, 2019 via email

Uh oh!

Timo-DK commented May 26, 2019

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented May 27, 2019

Build succeeded

Uh oh!

Uh oh!

Timo-DK commented May 25, 2019 •

edited

Loading

cormac-obrien left a comment •

edited

Loading