Summary: | [CMake] JSC shell sources should include JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Konstantin Tokarev <annulen> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Minor | CC: | achristensen, commit-queue, gyuyoung.kim, lforschler, ossy | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Local Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Konstantin Tokarev
2016-01-03 09:09:53 PST
Created attachment 268140 [details]
Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES)
Comment on attachment 268140 [details]
Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES)
LGTM.
Comment on attachment 268140 [details]
Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES)
Sorry for breaking this!
I am going to give cq- since this same construct exists in PlatformWin.cmake in this directory. Please fix it there too!
(In reply to comment #3) > Comment on attachment 268140 [details] > Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) > > Sorry for breaking this! > > I am going to give cq- since this same construct exists in PlatformWin.cmake > in this directory. Please fix it there too! Do you mean that PlatformWin.cmake includes ${JavaScriptCore_INCLUDE_DIRECTORIES} ? http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/shell/PlatformWin.cmake#L1 If so, I agree to remove it in PlatformWin.cmake although I don't know this patch is tightly related to remove it. (In reply to comment #3) > Comment on attachment 268140 [details] > Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) > > Sorry for breaking this! > > I am going to give cq- since this same construct exists in PlatformWin.cmake > in this directory. Please fix it there too! I'm not building for Windows, but for me it seems like it includes ${JavaScriptCore_INCLUDE_DIRECTORIES} twice, first time in CMakeLists.txt and second time in PlatformWin.cmake. Other port don't do it (In reply to comment #5) > (In reply to comment #3) > > Comment on attachment 268140 [details] > > Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) > > > > Sorry for breaking this! > > > > I am going to give cq- since this same construct exists in PlatformWin.cmake > > in this directory. Please fix it there too! > > I'm not building for Windows, but for me it seems like it includes > ${JavaScriptCore_INCLUDE_DIRECTORIES} twice, first time in CMakeLists.txt > and second time in PlatformWin.cmake. Other port don't do it yes, we meant it needs to be fixed in this patch together. (In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #3) > > > Comment on attachment 268140 [details] > > > Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) > > > > > > Sorry for breaking this! > > > > > > I am going to give cq- since this same construct exists in PlatformWin.cmake > > > in this directory. Please fix it there too! > > > > I'm not building for Windows, but for me it seems like it includes > > ${JavaScriptCore_INCLUDE_DIRECTORIES} twice, first time in CMakeLists.txt > > and second time in PlatformWin.cmake. Other port don't do it > > yes, we meant it needs to be fixed in this patch together. Shouldn't JavaScriptCore_INCLUDE_DIRECTORIES be removed from PlatformWin.cmake instead? (In reply to comment #7) > Shouldn't JavaScriptCore_INCLUDE_DIRECTORIES be removed from > PlatformWin.cmake instead? No, it's needed because in PlatformWin.cmake we make jscLib, which isn't needed in non-windows ports. jsc.exe is just a simple program that finds jscLib and starts it after possibly finding some other dlls. I don't think it's critical to include JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES on Windows. It wouldn't hurt, but I don't think it's needed. Commit this adding it to Windows or not, but don't remove JavaScriptCore_INCLUDE_DIRECTORIES from PlatformWin.cmake. Created attachment 268456 [details]
Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES)
Comment on attachment 268456 [details] Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES) Clearing flags on attachment: 268456 Committed r194705: <http://trac.webkit.org/changeset/194705> All reviewed patches have been landed. Closing bug. |