Bug 79344

Summary: [CMake] Add FindDirectX
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: New BugsAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, dbates, rakuco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 72816    
Attachments:
Description Flags
Patch
none
Patch aroben: review+, aroben: commit-queue-

Patrick R. Gansterer
Reported 2012-02-23 01:10:49 PST
[CMake] Add FindDirectX
Attachments
Patch (2.05 KB, patch)
2012-02-23 01:11 PST, Patrick R. Gansterer 		no flags
DetailsFormatted DiffDiff
Patch (2.19 KB, patch)
2012-02-23 10:13 PST, Patrick R. Gansterer 		aroben: review+
aroben: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Patrick R. Gansterer
Comment 1 2012-02-23 01:11:46 PST
Created attachment 128427 [details] Patch
Raphael Kubo da Costa (:rakuco)
Comment 2 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?
Patrick R. Gansterer
Comment 3 2012-02-23 10:13:55 PST
Created attachment 128492 [details] Patch
Raphael Kubo da Costa (:rakuco)
Comment 4 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.
Patrick R. Gansterer
Comment 5 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
Adam Roben (:aroben)
Comment 6 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.
Patrick R. Gansterer
Comment 7 2012-02-23 11:16:06 PST
Committed r108647: <http://trac.webkit.org/changeset/108647>
Note You need to log in before you can comment on or make changes to this bug.