Bug 79344 - [CMake] Add FindDirectX
Summary: [CMake] Add FindDirectX
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on:
Blocks: 72816
  Show dependency tree / graph
 
Reported: 2012-02-23 01:10 PST by Patrick R. Gansterer
Modified: 2012-02-23 11:16 PST (History)
3 users (show)

See Also:

Attachments
Patch (2.05 KB, patch)
2012-02-23 01:11 PST, Patrick R. Gansterer 		no flags Details | Formatted Diff | Diff
Patch (2.19 KB, patch)
2012-02-23 10:13 PST, Patrick R. Gansterer 		aroben: review+
aroben: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2012-02-23 01:10:49 PST 
[CMake] Add FindDirectX
Comment 1 Patrick R. Gansterer 2012-02-23 01:11:46 PST 
Created attachment 128427 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-02-23 05:53:36 PST 
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?
Comment 3 Patrick R. Gansterer 2012-02-23 10:13:55 PST 
Created attachment 128492 [details]
Patch
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-02-23 10:26:49 PST 
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.
Comment 5 Patrick R. Gansterer 2012-02-23 10:30:29 PST 
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
Comment 6 Adam Roben (:aroben) 2012-02-23 10:38:34 PST 
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.
Comment 7 Patrick R. Gansterer 2012-02-23 11:16:06 PST 
Committed r108647: <http://trac.webkit.org/changeset/108647>