Bug 177190 – JSC Xcode build should use unified sources for platform independent files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Keith Miller]

We should add core support for building unified source files to JSC. In the new architecture we platform independent source files should be added to Source/JavaScriptCore/unifiable-sources.txt. For platform specific files the old system remains in place, although in the near future that should change a bit.

[Keith Miller]

Created attachment 321238 [details]
WIP

[Keith Miller]

Created attachment 321242 [details]
WIP

[Keith Miller]

Created attachment 321251 [details]
Patch

[Keith Miller]

Created attachment 321252 [details]
Patch

[Saam Barati]

Comment on attachment 321252 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:18
> +        there are no spare files. If adding more bunle files becomes

bunle =>bundle

[Saam Barati]

Comment on attachment 321252 [details]
Patch

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

r=me
give me them fast builds

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:9520
> +			shellScript = "set -e\n\ncd $SRCROOT\n\nif [ \"${DEPLOYMENT_LOCATION}\" == \"Yes\" ]; then\n    BUILD_SCRIPTS_DIR=\"${SDKROOT}/usr/local/include/wtf/scripts\"\nelse\n    BUILD_SCRIPTS_DIR=\"${BUILT_PRODUCTS_DIR}/usr/local/include/wtf/scripts\"\nfi\n\nUnifiedSourceCppFileCount=133\n\n/usr/bin/env ruby \"${BUILD_SCRIPTS_DIR}/generate-unified-source-bundles.rb\" \"--derived-sources-path\" \"${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore\" \"--sources-file\" \"unifiable-sources.txt\" \"--max-cpp-bundle-count\" \"${UnifiedSourceCppFileCount}\" \"--max-obj-c-bundle-count\" \"0\"  > /dev/null\n";

It'd be great if we could dynamically compute --max-cpp-bundle-count. Perhaps open a bug somewhere to do this?

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:9787
> +				536B318B1F71C5990037FC33 /* UnifiedSource129.cpp in Sources */,

It'd be great if we could find a way to dynamically generate this. Maybe it's worth filing a bug somewhere so we don't forget about this annoyance of using unified builds.

> Source/WTF/scripts/generate-unified-source-bundles.rb:115
> +            raise "number of bundles for #{extension} sources, #{@bundleCount}, exceeded limit, #{@maxCount}. Please add #{bundleFileName} to Xcode"

Let's also make this print the variable thingy you need to change and the value you need to change it to via the Xcode GUI that passes into max-cpp-bundle-count

[Geoffrey Garen]

Comment on attachment 321252 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:23
> +        * unifiable-sources.txt: Added.

Do we plan to have some other source list that is not unifiable? If not (and I hope not), then we should just call this Sources.txt.

[Keith Miller]

Created attachment 321376 [details]
Patch

[Saam Barati]

Comment on attachment 321376 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:22
> +        LowLevelInterpreter.cpp can't be added to the unified source list yet
> +        due to a clang bug.

can you link to or create a bugzilla bug to track this so we don't forget to update things on our end.

> ChangeLog:20
> [phone]  Keith Miller  <[e-mail]>
> +
> +        JSC Xcode build should use unified sources for platform independent files
> +        https://bugs.webkit.org/show_bug.cgi?id=177190
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Source/cmake/WebKitMacros.cmake:
> +
> [phone]  Keith Miller  <[e-mail]>
> +
> +        JSC Xcode build should use unified sources for platform independent files
> +        https://bugs.webkit.org/show_bug.cgi?id=177190
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add a macro for collecting the set of "header" source files and adding the bundle
> +        files to the "sources" list.
> +
> +        * Source/cmake/WebKitMacros.cmake:

double changelog here.

[Keith Miller]

Created attachment 321381 [details]
Patch for landing

[Keith Miller]

Comment on attachment 321376 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:22
>> +        due to a clang bug.
> 
> can you link to or create a bugzilla bug to track this so we don't forget to update things on our end.

Done: https://bugs.webkit.org/show_bug.cgi?id=177277

>> ChangeLog:20
>> +        * Source/cmake/WebKitMacros.cmake:
> 
> double changelog here.

Whoops, Fixed.

[WebKit Commit Bot]

Comment on attachment 321381 [details]
Patch for landing

Clearing flags on attachment: 321381

Committed r222297: <http://trac.webkit.org/changeset/222297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Olivier Blin]

In generate-unified-source-bundles.rb, there is a typo in the directory name, a 'r' char is missing: "unified-souces"

[Keith Miller]

(In reply to Olivier Blin from comment #14)
> In generate-unified-source-bundles.rb, there is a typo in the directory
> name, a 'r' char is missing: "unified-souces"

Yeah, I noticed that and fixed it in: https://bugs.webkit.org/show_bug.cgi?id=177421

[Radar WebKit Bug Importer]

<rdar://problem/34693078>