Bug 177112 - JSC Xcode build should use unified sources
Summary: JSC Xcode build should use unified sources
Status: RESOLVED DUPLICATE of bug 177290
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on: 177190 177290
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-18 16:46 PDT by Keith Miller
Modified: 2017-09-24 18:44 PDT (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2017-09-18 16:46:15 PDT
We should also figure out how we are going to handle platform specific files. There are a couple of properties I think we like to have:

1) Adding files to one build system won't change the bundling of another build system
2) Adding source files should be relatively easy.
3) Minimize the build system changes.

1. or something very similar a hard requirement as it is important for helping to diagnose performance issues. There are several possible approaches we can take to listing unifiable sources in WebKit:

a) Build every .cpp file in all sub-directories. (requires every platform specific .cpp file has an ifdef)
b) Use a single source list file that uses C++ style macros to decide what files are to be built.
c) Use one source list file for each platform / platform independent files. (roughly matching the Platform*.cmake / CMakeLists.txt files) 
d) Have a script that runs to decide what files to include or not. (likely would need to be in the same language that bundler is in, currently ruby)
e) Only use source list files for Xcode.
f) Get CMake to provide the bundle/source list information to the Xcode build. (not sure if possible at all?)

I think there are a number of pros and cons to each option. 

a. 
  Pros:
    Developers no longer need to add files to the build list. 
  Cons:
    Source files will be added to the build regardless of whether or not they are needed.

b.
  Pros:
    People can add files based on feature defines.
  Cons:
    Tricky to make work with constraint 1. although it should be possible.
    Adds complexity the build system.
    Might cause weird issues with B&I.
  
c.
  Pros:
    Similar to the current build system.
    Doesn't require a lot of build system changes.
  Cons:
    ... dunno

d.
  Pros:
    Extremely flexible.
  Cons:
    Requires that everyone on WebKit needs to know the scripting language of choice.
    
e.
  Pros:
    Doesn't require changing the CMake build system.
  Cons:
    Likely thing will get out of sync.
    
f.
  Pros:
    Might make switch to a CMake only build system easier.
  Cons:
    Not clear if it's possible.
    Unknown amount of work.

My current opinion is that we should go with c. for now. Both because it should be the simplest to do and because it should provide all the robustness we need. While I like having a build system like b. it's more work to do and it's not clear that we can take advantage of it immediately. It also doesn't seem like it should be particularly hard to switch to something like b. later should we decide that's a better solution.

What do other people think?
Comment 1 Keith Miller 2017-09-18 16:56:06 PDT
I should also clarify that doing e. would be my preferred solution if we had a timetable for removing the Xcode build. The current current CMake solution works well, but I'm afraid that the builds will get out of sync and I'd really like there to only be one way of building WebKit for now. As a stopgap solution I think e. is the best solution but switching B&I to CMake seems far away. Another advantage with c. is that we can also switch back relatively quickly to the current CMake based lists if the Xcode build disappears.
Comment 2 Ryosuke Niwa 2017-09-18 17:21:21 PDT
So my hard requirement for an unified files is that for every platform P, source files A and B should be built in a single unified file C if there exists another unified file C' for some platform P' in which A and B are built together.

Basically, if A.cpp is built together with B.cpp in some platform, then it should be built together in all other platforms in which A.cpp and B.cpp are both built.

Without this property, we would have all sorts of weird crashes and performance bugs that only reproduce in a subset of platforms, and it would be impossible to debug/maintain our codebase.
Comment 3 Geoffrey Garen 2017-09-18 17:31:37 PDT
> So my hard requirement for an unified files is that for every platform P,
> source files A and B should be built in a single unified file C if there
> exists another unified file C' for some platform P' in which A and B are
> built together.

I agree that we want a close approximation of this requirement -- not just to avoid build-triggered performance and crash differences between platforms, which might be rare, but also to avoid build success/failure differences between platforms due to symbol conflicts or missing #includes.

I think it's OK not to satisfy this constraint 100% because doing so would be super costly. Darwin OS's include overlapping subsets of some platform-specific files, and I can't think of a way to specify perfectly replicated bundling of those subsets without maintaining an explicit bundling list separate from the build file list. Luckily, these subsets are small, so they probably aren't worth worrying about.

As a straw man proposal, I'd suggest just bundling all the platform-agnostic code in one pass, and then bundling the platform-specific code in a separate pass.

Relatedly, I think we'll need to bundle non-cpp code in separate passes as well, since bundled files must agree on language.
Comment 4 Geoffrey Garen 2017-09-18 17:42:40 PDT
> c) Use one source list file for each platform / platform independent files.
> (roughly matching the Platform*.cmake / CMakeLists.txt files) 

I tend to agree that (c) is the least bad option. It maps directly onto our existing CMake scripts because we can just replace the lists in those scripts with something like `cat ProjectFiles.txt` or `cat ProjectFilesMac.txt`. I might appreciate that change even if it had no other motivation. It makes small edits like adding, removing, and renaming files super easy, and separate from other build system logic.

(a) is interesting if you can make it work, but I suspect that making it work would be a non-trivial distraction from our main goal, and possibly a build time regression.

(b) has the downside that it complexifies the lists with #ifdefs, which are hard to read when they range over many lines and cover many platforms, and which add needless noise when you scroll past the macOS #ifdef just to add a file to the GTK build. But I'm open to (b).

(d) can have arbitrary upsides and downsides since it's Turing-complete, but it seems like an immediate downside would be to obfuscate the list of files being built, making it harder to edit.

(e) creates lots of possibilities for the Xcode build not matching the CMake build. It's a really important goal for these two builds to be as provably and idiot-proofly the same as we can make them.

(f) is interesting if you can make it work, but nobody knows how to make it work.

I think that (c) is not good enough if and only if we find a case where we need complex logic to decide if a given platform should build a given file. I'm not aware of any such case so far.
Comment 5 Keith Miller 2017-09-19 10:52:48 PDT
(In reply to Ryosuke Niwa from comment #2)
> So my hard requirement for an unified files is that for every platform P,
> source files A and B should be built in a single unified file C if there
> exists another unified file C' for some platform P' in which A and B are
> built together.
>
> Without this property, we would have all sorts of weird crashes and
> performance bugs that only reproduce in a subset of platforms, and it would
> be impossible to debug/maintain our codebase.

I don't see how your constraint givens you what you want. I think what you really want is property 1. Under your rule if I add a file to platform P platform P's bundles can all change. Why does this property give you anything more than what I described in constraint 1.? Property 1. means that patches cannot change the performance of a unrelated platform.

My plan is to do roughly what Geoff described where we bundle platform independent files then bundle specific files in a second pass.
Comment 6 Keith Miller 2017-09-24 18:44:48 PDT

*** This bug has been marked as a duplicate of bug 177290 ***