Bug 197174 - [CMake] Refactor WEBKIT_MAKE_FORWARDING_HEADERS into WEBKIT_COPY_FILES
Summary: [CMake] Refactor WEBKIT_MAKE_FORWARDING_HEADERS into WEBKIT_COPY_FILES
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords:
Depends on: 197559
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-22 13:01 PDT by Don Olmstead
Modified: 2019-05-03 09:12 PDT (History)
9 users (show)

See Also:


Attachments
Patch (14.42 KB, patch)
2019-05-01 18:15 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (15.92 KB, patch)
2019-05-01 18:27 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2019-04-22 13:01:23 PDT
WEBKIT_MAKE_FORWARDING_HEADERS should just copy files and not do anything else to the target framework.

Dependencies between the headers should be explicitly set. It is currently adding a dependency on the target framework. This shouldn't be the case.

Also this macro can be used to copy over other file types, such as scripts, that are currently just copied with file(COPY). If any of those files change then this isn't going to propagate which is a problem.
Comment 1 Don Olmstead 2019-05-01 18:15:12 PDT Comment hidden (obsolete)
Comment 2 Don Olmstead 2019-05-01 18:27:20 PDT
Created attachment 368746 [details]
Patch

Hopefully fix jsc only builds.
Comment 3 WebKit Commit Bot 2019-05-02 13:37:47 PDT
Comment on attachment 368746 [details]
Patch

Clearing flags on attachment: 368746

Committed r244881: <https://trac.webkit.org/changeset/244881>
Comment 4 WebKit Commit Bot 2019-05-02 13:37:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Zan Dobersek 2019-05-03 01:17:15 PDT
Comment on attachment 368746 [details]
Patch

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

> Source/JavaScriptCore/CMakeLists.txt:1308
> +add_dependencies(JavaScriptCorePrivateFrameworkHeaders JavaScriptCore)

Any reason why the dependency is reversed for JavaScriptCorePrivateFrameworkHeaders?
It's causing build failures for me on WPE, where CustomGlobalObjectClassTest.c from the testapi target is trying to include the JSObjectRefPrivate.h forwarding header which doesn't (yet?) exist.

Switching the targets fixes it for me.
Comment 6 WebKit Commit Bot 2019-05-03 07:27:50 PDT
Re-opened since this is blocked by bug 197559
Comment 7 Guillaume Emont 2019-05-03 07:32:41 PDT
(In reply to WebKit Commit Bot from comment #6)
> Re-opened since this is blocked by bug 197559

Rolling out because this breaks 3 EWS bots (jsc-armv7-ews, jsc-mips-ews and jsc-i386-ews) and would likely start to break buildbots once we do a clean build. The change suggested by Žan seems to fix it, but I don't know whether this would have ill side effects on other platforms, hence the rollout to give time to Dom to figure this out.
Comment 8 Don Olmstead 2019-05-03 09:12:44 PDT
(In reply to Zan Dobersek from comment #5)
> Comment on attachment 368746 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368746&action=review
> 
> > Source/JavaScriptCore/CMakeLists.txt:1308
> > +add_dependencies(JavaScriptCorePrivateFrameworkHeaders JavaScriptCore)
> 
> Any reason why the dependency is reversed for
> JavaScriptCorePrivateFrameworkHeaders?
> It's causing build failures for me on WPE, where
> CustomGlobalObjectClassTest.c from the testapi target is trying to include
> the JSObjectRefPrivate.h forwarding header which doesn't (yet?) exist.
> 
> Switching the targets fixes it for me.

I see the problem here but I'm not sure what the fix is so adding some JSC folks into the mix.

If you look in Source/JavaScriptCore/API/tests/CustomGlobalObjectClassTest.c theres

#include <JavaScriptCore/JSObjectRefPrivate.h>

If you look in ExecutionTimeLimitTest.cpp there's

#include "JSObjectRefPrivate.h"

I see a bunch of these inconsistencies in there. So yes that's why you saw this build failure.

JSC folks I would assume that testapi should just be using <JavaScriptCore/*.h> for includes. Is that a fair assessment?

Also should any of the other text executables and JSC itself be using <JavaScriptCore/*.h> style? I'm happy to fix this before relanding this patch.