Bug 79344 – [CMake] Add FindDirectX

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Patrick R. Gansterer]

[CMake] Add FindDirectX

[Patrick R. Gansterer]

Created attachment 128427 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 128427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128427&action=review

> Source/cmake/FindDirectX.cmake:12
> +    "$ENV{DXSDK_DIR}/Include"
> +    "C:/Program Files (x86)/Microsoft DirectX SDK*/Include"
> +    "C:/Program Files/Microsoft DirectX SDK*/Include"

I'd be more explicit and add the PATHS keyword before this hardcoded path list.

> Source/cmake/FindDirectX.cmake:15
> +SET(DirectX_ROOT_DIR "${DirectX_INCLUDE_DIRS}/..")

Isn't it cleaner to use GET_FILENAME_COMPONENT(DirectX_ROOT_DIR "${DirectX_INCLUDE_DIRS}/.." ABSOLUTE)?

> Source/cmake/FindDirectX.cmake:29
> +FIND_PACKAGE_HANDLE_STANDARD_ARGS(DirectX DEFAULT_MSG DirectX_LIBRARIES DirectX_INCLUDE_DIRS)

Shouldn't you also mark_as_advanced the user-visible variables?

[Patrick R. Gansterer]

Created attachment 128492 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 128492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128492&action=review

Looks OK to me.

> Source/cmake/FindDirectX.cmake:21
> +IF (CMAKE_CL_64)
> +    SET(DirectX_LIBRARY_PATHS "${DirectX_ROOT_DIR}/Lib/x64")
> +ELSE ()
> +    SET(DirectX_LIBRARY_PATHS "${DirectX_ROOT_DIR}/Lib/x86" "${DirectX_ROOT_DIR}/Lib")
> +ENDIF ()

Minor: I don't really remember if we were suppose to use IF() or IF () in our agreed CMake coding style.

[Patrick R. Gansterer]

Comment on attachment 128492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128492&action=review

>> Source/cmake/FindDirectX.cmake:21
>> +ENDIF ()
> 
> Minor: I don't really remember if we were suppose to use IF() or IF () in our agreed CMake coding style.

we have a space after an if in the c++ style too, so this matches WebKit style more

[Adam Roben (:aroben)]

Comment on attachment 128492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128492&action=review

> Source/cmake/FindDirectX.cmake:12
> +    "C:/Program Files (x86)/Microsoft DirectX SDK*/Include"
> +    "C:/Program Files/Microsoft DirectX SDK*/Include"

Should we use $ENV{PROGRAMFILES} and $ENV{PROGRAMFILES(X86)} here? Hard-coding "C:" doesn't seem so great.

[Patrick R. Gansterer]

Committed r108647: <http://trac.webkit.org/changeset/108647>