Bug 179814

Created attachment 327155 [details] Proposed fix I've tested this with WinCairo build and it was successful building.

After submitting I realized I intended to check that the build stops if the perl script returns an error. The perl script will return an error if a particular forwarding header already exists but has a different path than the expected path. This could happen if there are two header files in JavaScriptCore with the same name, which would be unexpected. I'll plan to submit a new patch.

Created attachment 327160 [details] Patch set #2 checks if the perl script to create forwarding headers returns an error; if so it causes the batch script to return an error and the build stops. hmmm... I realize my patch isn't formatted quite right. Some time later I suppose I'll have to figure out why prepare-ChangeLog is dying: D:\git\webkit-upstream>perl Tools\Scripts\prepare-ChangeLog --git-commit=HEAD~1...HEAD Running status to find changed, added, or removed files. Reviewing diff to determine which lines changed. Extracting affected function names from source files. Use of uninitialized value $first_line in pattern match (m//) at Tools\Scripts\prepare-ChangeLog line 765. Change author: Mark Salisbury <mark.salisbury@hp.com>. Editing the Source\JavaScriptCore\ChangeLog file. -- Please remember to include a detailed description in your ChangeLog entry. -- -- See <http://webkit.org/coding/contributing.html> for more info -- I found a bug in webkit-patch if SVN is not installed... I'll have to submit a fix for that.

(In reply to Mark Salisbury from comment #3) > I found a bug in webkit-patch if SVN is not installed... I'll have to > submit a fix for that. Not sure if that is a bug. The WebKit repository uses SVN. This means that if you want to use git and use the developer tools for sending patches and committing you need to configure also the git-svn integration as described https://webkit.org/getting-the-code/#checking-out-with-git

I think this patch will be rejected because of what is said in my quote. PlatformWin.cmake is used by both WinCairo and AppleWin, and it's nothing like generic support of platform Windows in WebKit, no, it's port-specific build system like PlatformGTK.cmake or PlatformWPE.cmake

If patch is changed to be WinCairo-specific it might be accepted if Sony folks have nothing against it

(In reply to Carlos Alberto Lopez Perez from comment #4) > (In reply to Mark Salisbury from comment #3) > > I found a bug in webkit-patch if SVN is not installed... I'll have to > > submit a fix for that. > > Not sure if that is a bug. The WebKit repository uses SVN. This means that > if you want to use git and use the developer tools for sending patches and > committing you need to configure also the git-svn integration as described > https://webkit.org/getting-the-code/#checking-out-with-git Interesting. Seems like SVN should only be required if you want to commit directly to SVN. I read this page: https://trac.webkit.org/wiki/UsingGitWithWebKit and while it doesn't say you don't need to install SVN, it says tools have been updated to work with a GIT workflow.

(In reply to Konstantin Tokarev from comment #5) > I think this patch will be rejected because of what is said in my quote. > PlatformWin.cmake is used by both WinCairo and AppleWin, and it's nothing > like generic support of platform Windows in WebKit, no, it's port-specific > build system like PlatformGTK.cmake or PlatformWPE.cmake Right. What I'm ultimately working on is running WPE on Windows, but I'm not trying to upstream anything directly related to that yet. I think fundamentally that making copies of headers is going to cause headaches down the road, if it hasn't already (for folks besides me).

Comment on attachment 327160 [details] Patch set #2 checks if the perl script to create forwarding headers returns an error; if so it causes the batch script to return an error and the build stops. View in context: https://bugs.webkit.org/attachment.cgi?id=327160&action=review > Source/JavaScriptCore/PlatformWin.cmake:44 > +file(WRITE "${JavaScriptCore_POST_BUILD_COMMAND}" "perl ${JAVASCRIPTCORE_DIR}/Scripts/make-forwarding-headers.pl \"${DERIVED_SOURCES_DIR}/JavaScriptCore/*.h\" \"${DERIVED_SOURCES_DIR}/JavaScriptCore\" \"${FORWARDING_HEADERS_DIR}/JavaScriptCore\"\n") I think better approach would be to pass list of globs as arguments to one script invocation, and ditch preBuild.cmd file altogether, replacing JavaScriptCore_PRE_BUILD_COMMAND with script invocation At very least use && to join commands instead of "if %errorlevel% NEQ 0 exit /b 1\n" > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:1 > +use strict; It's also a good practice to add "use warnings" to all scripts, unfortunately many our scripts don't > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:5 > +my $searchPath = shift; you'll need Getopt::Long > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:10 > +foreach(@headers) { use explicit name for iterator > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:12 > + my $forwardingHeader = $forwardingDir.'/'.$file; use File::Spec->join > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:15 > + open (HEADER, $forwardingHeader) or die("Unable to open $forwardingHeader"); Barewords for file handles are bad practice, use my $header Don't forget to add $! into your die messages open without '<' is fragile, if it happens that file name starts with special character it may lead to unexpected result > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:21 > + print "Creating forwarding header $forwardingHeader with #include \"$includePath/$file\"\n"; Don't duplicate $fileContent here, chomp it, or add "\n" in the next statement > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:24 > + close(HEADER); Better check errors in close too. This may not be incredibly useful for files, but for other types of handles it's really needed (e.g. pipes). Just keep in mind

(In reply to Konstantin Tokarev from comment #9) > Comment on attachment 327160 [details] > Patch set #2 checks if the perl script to create forwarding headers returns > an error; if so it causes the batch script to return an error and the build > stops. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327160&action=review > > > Source/JavaScriptCore/PlatformWin.cmake:44 > > +file(WRITE "${JavaScriptCore_POST_BUILD_COMMAND}" "perl ${JAVASCRIPTCORE_DIR}/Scripts/make-forwarding-headers.pl \"${DERIVED_SOURCES_DIR}/JavaScriptCore/*.h\" \"${DERIVED_SOURCES_DIR}/JavaScriptCore\" \"${FORWARDING_HEADERS_DIR}/JavaScriptCore\"\n") > > I think better approach would be to pass list of globs as arguments to one > script invocation, and ditch preBuild.cmd file altogether, replacing > JavaScriptCore_PRE_BUILD_COMMAND with script invocation That was my approach on my first pass. I had trouble with CMake and multiple lines... I could give it another shot though. > > At very least use && to join commands instead of "if %errorlevel% NEQ 0 exit > /b 1\n" > > > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:1 > > +use strict; > > It's also a good practice to add "use warnings" to all scripts, > unfortunately many our scripts don't > > > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:5 > > +my $searchPath = shift; > > you'll need Getopt::Long > > > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:10 > > +foreach(@headers) { > > use explicit name for iterator > > > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:12 > > + my $forwardingHeader = $forwardingDir.'/'.$file; > > use File::Spec->join > > > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:15 > > + open (HEADER, $forwardingHeader) or die("Unable to open $forwardingHeader"); > > Barewords for file handles are bad practice, use my $header > > Don't forget to add $! into your die messages > > open without '<' is fragile, if it happens that file name starts with > special character it may lead to unexpected result > > > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:21 > > + print "Creating forwarding header $forwardingHeader with #include \"$includePath/$file\"\n"; > > Don't duplicate $fileContent here, chomp it, or add "\n" in the next > statement > > > Source/JavaScriptCore/Scripts/make-forwarding-headers.pl:24 > > + close(HEADER); > > Better check errors in close too. This may not be incredibly useful for > files, but for other types of handles it's really needed (e.g. pipes). Just > keep in mind Great suggestions. perl isn't my strongest area.

OK, on the first suggestion - scrap the preBuild.cmd on postBuild.cmd scripts - I tried again. I'm running into a problem with quotes and I think *maybe* this is why these were written as .cmd files in the first place. CMake uses one set of rules for quoting strings when writing to a file, which seem sane, but for building up a string directly quotes get wonky fast. For example, a line like this: set(var "${var}here is some text \"now it is quoted\"") ends up leaving the quotes at the beginning and end of the line. and the escapes are also left in. I also tried this: set(var ${var}here is some text "now it is quoted") this takes quotes out completely. And this: set(var ${var}here is some text \"now it is quoted\") leaves the escapes in. So I'm going to stick with .cmd files unless someone can show me an example of how to concatenate strings with quotes successfully. I'm thinking not to '&&' all the script invocations together because there is a limit on how many characters you can have for a single command. I read it's 8192 chars and we're very close to that. As far as globbing all the dirs together for a single invocation, I'm trying to do what I think is a little easier to read. It seems to execute plenty fast with multiple invocations.

Created attachment 327163 [details] Proposed fix I believe I addressed everything you requested in the perl script with the exception of: "Don't duplicate $fileContent here, chomp it, or add "\n" in the next statement" I wasn't sure how to interpret that...

I mean just print chomp($fileContent) instead of duplicating "#include...", or don't add trailing \n when $fileContent is assigned, and do print $header $fileContent, "\n" in the next line

Actually I don't think you need to mess with this code at all, if you are only interested in WPE

(In reply to Konstantin Tokarev from comment #14) > Actually I don't think you need to mess with this code at all, if you are > only interested in WPE While brining up a WPE build on Windows we discovered that we needed the forwarding headers for JavaScriptCore. We copied the same xcopy code that Windows builds use. Then later I started getting zillions of compile errors from multiply defined symbols. Some even caused compiler crashes. So I analyzed the problem, decided the root of the problem was exactly what this bug says. As far as how we might introduce a WPE build on Windows, it'd be nice to avoid copying the code but perhaps locating common OS(WINDOWS) code to one place. Even if we don't do that, I think it still is logical to fix this for Windows builds too. I think they will likely see it sooner or later. If you guys don't believe this is fundamentally a potential problem (of copying headers), then we can close this bug. Thanks for your time so far!

Created attachment 327166 [details] Proposed fix OK, got it. Removed the trailing \n.

WPE uses Source/WebKit/Scripts/generate-forwarding-headers.pl which should generate all needed forwarding headers (maybe you need to add --platform win there though)

(In reply to Konstantin Tokarev from comment #17) > WPE uses Source/WebKit/Scripts/generate-forwarding-headers.pl which should > generate all needed forwarding headers (maybe you need to add --platform win > there though) PlatformWin, GTK, and WPE all use that script. You have a good point though. Why was the xcopy forwarding header script ever added in the first place? I'll look at the history when this was introduced. It produces far more forwarding headers for JavaSriptCore than Source/WebKit/Scripts/generate-forwarding-headers.pl does... Until I have an answer I would suggest not taking this patch. I think there's still a problem here (xcopy causes problems) but I'm not sure I'm solving it in the right way. Is there a way to clear review? commit-queue?

(In reply to Mark Salisbury from comment #18) > (In reply to Konstantin Tokarev from comment #17) > > WPE uses Source/WebKit/Scripts/generate-forwarding-headers.pl which should > > generate all needed forwarding headers (maybe you need to add --platform win > > there though) > > PlatformWin, GTK, and WPE all use that script. Out of tree Qt port (which I'm developing) also uses it (on Windows too). > > You have a good point though. Why was the xcopy forwarding header script > ever added in the first place? I'll look at the history when this was > introduced. Because AppleWin needs to support separate building of WTF, WebCore, and WebKit, they want to copy all header files. > > It produces far more forwarding headers for JavaSriptCore than > Source/WebKit/Scripts/generate-forwarding-headers.pl does... Just as planned. > > Until I have an answer I would suggest not taking this patch. I think > there's still a problem here (xcopy causes problems) but I'm not sure I'm > solving it in the right way. > > Is there a way to clear review? commit-queue? Same way as you set them

(In reply to Mark Salisbury from comment #0) > When forwarding headers are copies of header files, and both the forwarding > header version and the real version of the header file are included, compile > errors arise from symbols being defined multiple times. > > Instead of copying the header file we should create a forwarding header that > contains a #include path to the real header. The copy command is unique to > the Windows... > > Konstantin Tokarev said in reply (see to mail subject "[webkit-dev] unified > sources build + forwarding headers that are copies") : > > "AFAIK Windows uses different approach because AppleWin needs to support > separate building of WTF, WebCore, and WebKit." It’s my understanding that the copy is present in ALL Apple ports. I think it’s better for the project as a whole for everyone to move to copying the headers for consistency.

Note: Apple's CMake build does not copy the headers.

(In reply to Michael Catanzaro from comment #21) > Note: Apple's CMake build does not copy the headers. But it’s not shipping anything. Is there any reason why we shouldn’t just copy the headers in CMake ports?

>Is there any reason why we shouldn’t just copy the headers in CMake ports? You mean avoid copying? Because doing otherwise is a regression and leads to possible #pragma once problems described by Mark.

(In reply to Konstantin Tokarev from comment #23) > >Is there any reason why we shouldn’t just copy the headers in CMake ports? > > You mean avoid copying? Because doing otherwise is a regression and leads to > possible #pragma once problems described by Mark. I'm saying why don't we fix what's wrong within CMake or whatever else is happening to cause the #pragma once issue so it is consistent with what Apple is doing when they just copy the headers. If we think of WTF, JSC, etc as something that can be built independently then at the end we would have an "install" step that would put the headers where they need to be so WebCore, WebKit, etc can build.

(In reply to Don Olmstead from comment #24) > (In reply to Konstantin Tokarev from comment #23) > > >Is there any reason why we shouldn’t just copy the headers in CMake ports? > > > > You mean avoid copying? Because doing otherwise is a regression and leads to > > possible #pragma once problems described by Mark. > > I'm saying why don't we fix what's wrong within CMake or whatever else is > happening to cause the #pragma once issue so it is consistent with what > Apple is doing when they just copy the headers. Copying headers is fundamentally incompatible with #pragma once. The real question here is: why does Apple Windows build at all? I would expect it to be broken too.

(In reply to Michael Catanzaro from comment #25) > (In reply to Don Olmstead from comment #24) > > (In reply to Konstantin Tokarev from comment #23) > > > >Is there any reason why we shouldn’t just copy the headers in CMake ports? > > > > > > You mean avoid copying? Because doing otherwise is a regression and leads to > > > possible #pragma once problems described by Mark. > > > > I'm saying why don't we fix what's wrong within CMake or whatever else is > > happening to cause the #pragma once issue so it is consistent with what > > Apple is doing when they just copy the headers. > > Copying headers is fundamentally incompatible with #pragma once. > > The real question here is: why does Apple Windows build at all? I would > expect it to be broken too. Not everything is using #pragma once. See https://bugs.webkit.org/show_bug.cgi?id=174986 where some wtf headers aren't working. Then there's https://bugs.webkit.org/show_bug.cgi?id=177202. These are all fundamentally the same bug. If you want to get an idea on what is and isn't using #pragma once https://github.com/cgmb/guardonce is a nice python utility. Its my understanding that Apple has a HARD requirement that each library be able to be built independently. There is also the requirement that if a library can be built as a framework then the headers need to be flat, eg #include <WebCore/Foo.h>. That is the only reason pal and wtf are not flat since they're built statically. Internally we've been looking at how to solve this with copying for all CMake ports. We don't have anything completed yet but we are hitting the same issue Mark is hitting with WebKit(2) support on WinCairo.

I believe that both Windows ports (AppleWin, WinCairo) have the same xcopy logic to create forwarding headers. Source\JavaScriptCore\PlatformWin.cmake is used by both CMake builds. The copy was introduced here: https://bugs.webkit.org/show_bug.cgi?id=148198 "CMake Windows build should not include files directly from other Source direc..." At that time I don't think #pragma once was being used. The rationale was as has been mentioned so that the next projects (frameworks) could include previous frameworks with a simple #include <JavaScriptCore/SomeJSFile.h>. There's no careful selection of what public .h files should be, and it'd be hard to do so anyway since one .h file can lead to many others, sometimes ones you don't expect. So here's where I am with this now. My current patch has an incompatibility with WebKit/Scripts/generate-forwarding-headers.pl. They both try to create forwarding headers for some of the same JavaScriptCore files. I print the full path to the .h file and WebKit/Scripts/generate-forwarding-headers.pl lists out a relative path. WebKit/Scripts/generate-forwarding-headers.pl overwrites that path that the script in this patch products. The reason I went with the full path is that I saw a problem with infinite recursion of a forwarding header include. The forwarding header include path came before the real directory, so once an include started on a forwarding header, it tried to include the exact same file again. A #pragma once in the forwarding header would prevent the re-inclusion, however, it would also fail to include the real .h file. The infinite recursion to me means that there is some ordering difference in header search paths between my build and WinCairo build. I see a couple of options, if I am to pursue preventing copying of .h files (which I hope others agree fundamentally doesn't make sense): 1) Modify WebKit/Scripts/generate-forwarding-headers.pl so that it prints the full path to the real header. Then the order of header search paths doesn't matter. This does make the derived sources output (forwarding headers) not relocatable. I don't know if it needs to be. I believe these .h files are just used in an internal build. The real public .h files from WebKit are JavaScriptCore API and WebKit API files. 2) Figure out why my Win WPE build has a different header search path order than WinCairo. The forwarding header include path has to come AFTER the include search path that triggers finding the real .h file. In practice I think that means the forwarding header path should come last. Thoughts? Thanks everyone. I've been a downstream user of WebKit for quite a while now and feel bad that I haven't been able to give back more. I'm hoping to be a better citizen as we are now working on updating our WebKit version (used in HP products).

(In reply to Mark Salisbury from comment #27) > 2) Figure out why my Win WPE build has a different header search path order > than WinCairo. The forwarding header include path has to come AFTER the > include search path that triggers finding the real .h file. In practice I > think that means the forwarding header path should come last. It doesn't have a different search order. WinCairo has ForwardingHeaders first too. This wasn't a problem when the files were copies. I still believe the same options apply though: 1) Make the forwarding header paths full paths 2) Move fowarding header include path to the end of the includes paths I like #1 better. For the curious, here's the example I included in e-mail about how the same header file gets included twice. There were many others: First source file including ArrayBufferSharingMode.h: 1>Note: including file: D:\git\webkit-org\Source\WebCore\css/DOMMatrix.cpp 1>Note: including file: D:\git\webkit-org\Source\WebCore\config.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\DOMMatrix.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\DOMMatrixReadOnly.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\DOMMatrixInit.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\DOMMatrix2DInit.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\bindings\js\ScriptWrappable.h 1>Note: including file: D:\git\webkit-org\Source\JavaScriptCore\heap/Weak.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\platform\graphics\transforms\TransformationMatrix.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\platform\graphics\FloatPoint.h 1>Note: including file: d:\git\webkit-org\source\webcore\platform\graphics\FloatSize.h 1>Note: including file: d:\git\webkit-org\source\webcore\platform\graphics\IntPoint.h 1>Note: including file: d:\git\webkit-org\source\webcore\platform\graphics\IntSize.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\platform\graphics\FloatPoint3D.h 1>Note: including file: D:\git\webkit-org\Source\JavaScriptCore\runtime/Float32Array.h 1>Note: including file: d:\git\webkit-org\source\javascriptcore\runtime\TypedArrays.h 1>Note: including file: d:\git\webkit-org\source\javascriptcore\runtime\GenericTypedArrayView.h 1>Note: including file: d:\git\webkit-org\source\javascriptcore\runtime\ArrayBuffer.h 1>Note: including file: d:\git\webkit-org\source\javascriptcore\runtime\ArrayBufferSharingMode.h Second source file including ArrayBufferSharingMode.h: 1>Note: including file: D:\git\webkit-org\Source\WebCore\css/DOMMatrixReadOnly.cpp 1>Note: including file: D:\git\webkit-org\Source\WebCore\config.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\CSSToLengthConversionData.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\dom\DOMPoint.h 1>Note: including file: d:\git\webkit-org\source\webcore\dom\DOMPointReadOnly.h 1>Note: including file: d:\git\webkit-org\source\webcore\dom\DOMPointInit.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\TransformFunctions.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\platform\graphics\transforms\TransformOperations.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\platform\graphics\LayoutSize.h 1>Note: including file: d:\git\webkit-org\source\webcore\platform\graphics\transforms\TransformOperation.h 1>Note: including file: D:\git\webkit-org\WebKitBuild\Debug\DerivedSources\ForwardingHeaders\JavaScriptCore/GenericTypedArrayViewInlines.h 1>Note: including file: d:\git\webkit-org\webkitbuild\debug\derivedsources\forwardingheaders\javascriptcore\GenericTypedArrayView.h 1>Note: including file: d:\git\webkit-org\webkitbuild\debug\derivedsources\forwardingheaders\javascriptcore\ArrayBuffer.h 1>Note: including file: d:\git\webkit-org\webkitbuild\debug\derivedsources\forwardingheaders\javascriptcore\ArrayBufferSharingMode.h

(In reply to Mark Salisbury from comment #28) > (In reply to Mark Salisbury from comment #27) > > > 2) Figure out why my Win WPE build has a different header search path order > > than WinCairo. The forwarding header include path has to come AFTER the > > include search path that triggers finding the real .h file. In practice I > > think that means the forwarding header path should come last. > > It doesn't have a different search order. WinCairo has ForwardingHeaders > first too. This wasn't a problem when the files were copies. > > I still believe the same options apply though: > > 1) Make the forwarding header paths full paths > 2) Move fowarding header include path to the end of the includes paths > > I like #1 better. > > For the curious, here's the example I included in e-mail about how the same > header file gets included twice. There were many others: > > First source file including ArrayBufferSharingMode.h: > > 1>Note: including file: D:\git\webkit-org\Source\WebCore\css/DOMMatrix.cpp > 1>Note: including file: D:\git\webkit-org\Source\WebCore\config.h > 1>Note: including file: d:\git\webkit-org\source\webcore\css\DOMMatrix.h > 1>Note: including file: > d:\git\webkit-org\source\webcore\css\DOMMatrixReadOnly.h > 1>Note: including file: > d:\git\webkit-org\source\webcore\css\DOMMatrixInit.h > 1>Note: including file: > d:\git\webkit-org\source\webcore\css\DOMMatrix2DInit.h > 1>Note: including file: > D:\git\webkit-org\Source\WebCore\bindings\js\ScriptWrappable.h > 1>Note: including file: > D:\git\webkit-org\Source\JavaScriptCore\heap/Weak.h > 1>Note: including file: > D:\git\webkit- > org\Source\WebCore\platform\graphics\transforms\TransformationMatrix.h > 1>Note: including file: > D:\git\webkit-org\Source\WebCore\platform\graphics\FloatPoint.h > 1>Note: including file: > d:\git\webkit-org\source\webcore\platform\graphics\FloatSize.h > 1>Note: including file: > d:\git\webkit-org\source\webcore\platform\graphics\IntPoint.h > 1>Note: including file: > d:\git\webkit-org\source\webcore\platform\graphics\IntSize.h > 1>Note: including file: > D:\git\webkit-org\Source\WebCore\platform\graphics\FloatPoint3D.h > 1>Note: including file: > D:\git\webkit-org\Source\JavaScriptCore\runtime/Float32Array.h > 1>Note: including file: > d:\git\webkit-org\source\javascriptcore\runtime\TypedArrays.h > 1>Note: including file: > d:\git\webkit-org\source\javascriptcore\runtime\GenericTypedArrayView.h > 1>Note: including file: > d:\git\webkit-org\source\javascriptcore\runtime\ArrayBuffer.h > 1>Note: including file: > d:\git\webkit-org\source\javascriptcore\runtime\ArrayBufferSharingMode.h > > Second source file including ArrayBufferSharingMode.h: > > 1>Note: including file: > D:\git\webkit-org\Source\WebCore\css/DOMMatrixReadOnly.cpp > 1>Note: including file: D:\git\webkit-org\Source\WebCore\config.h > 1>Note: including file: > d:\git\webkit-org\source\webcore\css\CSSToLengthConversionData.h > 1>Note: including file: D:\git\webkit-org\Source\WebCore\dom\DOMPoint.h > 1>Note: including file: > d:\git\webkit-org\source\webcore\dom\DOMPointReadOnly.h > 1>Note: including file: > d:\git\webkit-org\source\webcore\dom\DOMPointInit.h > 1>Note: including file: > d:\git\webkit-org\source\webcore\css\TransformFunctions.h > 1>Note: including file: > D:\git\webkit- > org\Source\WebCore\platform\graphics\transforms\TransformOperations.h > 1>Note: including file: > D:\git\webkit-org\Source\WebCore\platform\graphics\LayoutSize.h > 1>Note: including file: > d:\git\webkit- > org\source\webcore\platform\graphics\transforms\TransformOperation.h > 1>Note: including file: > D:\git\webkit- > org\WebKitBuild\Debug\DerivedSources\ForwardingHeaders\JavaScriptCore/ > GenericTypedArrayViewInlines.h > 1>Note: including file: > d:\git\webkit- > org\webkitbuild\debug\derivedsources\forwardingheaders\javascriptcore\Generic > TypedArrayView.h > 1>Note: including file: > d:\git\webkit- > org\webkitbuild\debug\derivedsources\forwardingheaders\javascriptcore\ArrayBu > ffer.h > 1>Note: including file: > d:\git\webkit- > org\webkitbuild\debug\derivedsources\forwardingheaders\javascriptcore\ArrayBu > fferSharingMode.h What if within the JavaScriptCore/CMakeLists.txt if you change JavaScriptCore_INCLUDE_DIRECTORIES into JavaScriptCore_PRIVATE_INCLUDE_DIRECTORIES? Then those include directories shouldn't propagate to WebCore and you shouldn't end up in d:\git\webkit-org\source\javascriptcore\runtime\ArrayBufferSharingMode.h.

Then we again end up with duplicated include directory listings in each CMakeLists.txt

It makes sense to make them private if headers are copied though

(In reply to Don Olmstead from comment #20) > It’s my understanding that the copy is present in ALL Apple ports. I think > it’s better for the project as a whole for everyone to move to copying the > headers for consistency. I agree. (In reply to Michael Catanzaro from comment #25) > Copying headers is fundamentally incompatible with #pragma once. This is not true if we include directories well. For example, when building WTF we should include the Source/WTF directory and not the directory containing the copies and when building everything else we should include the directory containing the copies. When building JSC we should include Source/JavaScriptCore and not the directory containing the copies and when building everything else we should include the directory containing the copies. Etc.

> What if within the JavaScriptCore/CMakeLists.txt if you change > JavaScriptCore_INCLUDE_DIRECTORIES into > JavaScriptCore_PRIVATE_INCLUDE_DIRECTORIES? Then those include directories > shouldn't propagate to WebCore and you shouldn't end up in > d:\git\webkit-org\source\javascriptcore\runtime\ArrayBufferSharingMode.h. I'll try that.

(In reply to Alex Christensen from comment #32) > (In reply to Don Olmstead from comment #20) > > It’s my understanding that the copy is present in ALL Apple ports. I think > > it’s better for the project as a whole for everyone to move to copying the > > headers for consistency. > > I agree. > > (In reply to Michael Catanzaro from comment #25) > > Copying headers is fundamentally incompatible with #pragma once. > > This is not true if we include directories well. For example, when building > WTF we should include the Source/WTF directory and not the directory > containing the copies and when building everything else we should include > the directory containing the copies. When building JSC we should include > Source/JavaScriptCore and not the directory containing the copies and when > building everything else we should include the directory containing the > copies. Etc. Sounds like that would work. I do think having copies of .h files in different places is kludgy. As long as the discussion is around what's ideal, wouldn't it be better not to need forwarding headers at all? What if all #includes were relative to either the top level source directory or to the derived sources directory? This might even make compiles a little bit faster since they wouldn't have to search so many directories. It'd make build output files smaller ;) It would make code more readable. What problems would arise from this?

(In reply to Mark Salisbury from comment #34) > (In reply to Alex Christensen from comment #32) > > (In reply to Don Olmstead from comment #20) > > > It’s my understanding that the copy is present in ALL Apple ports. I think > > > it’s better for the project as a whole for everyone to move to copying the > > > headers for consistency. > > > > I agree. > > > > (In reply to Michael Catanzaro from comment #25) > > > Copying headers is fundamentally incompatible with #pragma once. > > > > This is not true if we include directories well. For example, when building > > WTF we should include the Source/WTF directory and not the directory > > containing the copies and when building everything else we should include > > the directory containing the copies. When building JSC we should include > > Source/JavaScriptCore and not the directory containing the copies and when > > building everything else we should include the directory containing the > > copies. Etc. > > Sounds like that would work. I do think having copies of .h files in > different places is kludgy. > > As long as the discussion is around what's ideal, wouldn't it be better not > to need forwarding headers at all? > > What if all #includes were relative to either the top level source directory > or to the derived sources directory? This might even make compiles a little > bit faster since they wouldn't have to search so many directories. It'd > make build output files smaller ;) It would make code more readable. > > What problems would arise from this? https://bugs.webkit.org/show_bug.cgi?id=180063#c5

Apple has a requirement for forwarding headers to be copies: https://bugs.webkit.org/show_bug.cgi?id=180063#c5 The problem I'm running into can likely be addressed by ensuring that PRIVATE headers aren't leaked into the next project's include paths.