Bug 152664 – [CMake] JSC shell sources should include JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Konstantin Tokarev]

JSC/CMskeLists.txt includes both JavaScriptCore_INCLUDE_DIRECTORIES and JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES, and JSC/shell/CMakeLists.txt should do it as well.

In my build setup ICU include paths happen to be in "system" include path, so JSC library builds fine and shells don't

[Konstantin Tokarev]

Created attachment 268140 [details]
Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES)

[Gyuyoung Kim]

Comment on attachment 268140 [details]
Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES)

LGTM.

[Michael Catanzaro]

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!

[Gyuyoung Kim]

(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.

[Konstantin Tokarev]

(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

[Gyuyoung Kim]

(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.

[Konstantin Tokarev]

(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?

[Alex Christensen]

(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.

[Konstantin Tokarev]

Created attachment 268456 [details]
Added include_directories(JavaScriptCore_SYSTEM_INCLUDE_DIRECTORIES)

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.