Unified sources have broken our #include hygiene

# Simon Fraser (12 days ago)

Unified sources allow you to get away without #including all the files you need in a .cpp file, because some earlier file in the group has probably already included them for you.

This means that when you add a new file to the build, and the unified sources get shuffled around, you end up with a long list of build breakages, some platform-specific, that you can only fix by repeating EWS trials. Here's an example: bugs.webkit.org/show_bug.cgi?id=189223. That patch added on new file in Source/WebCore/rendering, which required unrelated #include changes in at least:

rendering/RenderBlockFlow.cpp: rendering/RenderFrame.cpp: rendering/RenderImage.cpp:

How can we solve this? Should we have an EWS that builds non-unified? Can we somehow have the style checker do #include enforcement?

Simon

Contact us to advertise here
# Tim Horton (12 days ago)

On Sep 1, 2018, at 17:14, Simon Fraser <simon.fraser at apple.com> wrote:

Unified sources allow you to get away without #including all the files you need in a .cpp file, because some earlier file in the group has probably already included them for you.

This means that when you add a new file to the build, and the unified sources get shuffled around, you end up with a long list of build breakages, some platform-specific, that you can only fix by repeating EWS trials. Here's an example: bugs.webkit.org/show_bug.cgi?id=189223. That patch added on new file in Source/WebCore/rendering, which required unrelated #include changes in at least:

rendering/RenderBlockFlow.cpp: rendering/RenderFrame.cpp: rendering/RenderImage.cpp:

How can we solve this? Should we have an EWS that builds non-unified?

Yes!

Can we somehow have the style checker do #include enforcement?

Certainly not the style checker we have now!

# Darin Adler (12 days ago)

On Sep 1, 2018, at 5:14 PM, Simon Fraser <simon.fraser at apple.com> wrote:

Unified sources allow you to get away without #including all the files you need in a .cpp file, because some earlier file in the group has probably already included them for you.

This means that when you add a new file to the build, and the unified sources get shuffled around, you end up with a long list of build breakages, some platform-specific, that you can only fix by repeating EWS trials.

Yes.

We knew this was going to happen when evaluated the proposal before we switched to unified sources. I believe you can find some discussion of it in webkit-dev.

How can we solve this?

I don’t think we should try to solve it.

To me at the moment, this seems to be a minor irritation. Even without unified sources, it’s common to get includes wrong for platforms other than the one you are testing on and find this out only when building on EWS.

I would be OK having an EWS server that builds various platforms without unified sources, but while it might help it might also hurt, adding more EWS results to interpret for each patch, and also finding theoretical problems that don’t affect any platform.

If there’s a tool that can figure out what files need to be included by analyzing source code, and that works well enough to be practical, I’d love to arrange it so that we can use that instead of having programmers have to write their own include statements. I’ve long been interested in <https://include-what-you-use.org <https://include-what-you-use.org/>> and wondered whether we could use it, but it sort of does the opposite, and the “multiple platforms” problem could well make even that tool impractical for us.

— Darin

# Geoffrey Garen (11 days ago)

FWIW, this problem is caused by unified sources and precompiled headers.

Unified sources allow you to get away without #including all the files you need in a .cpp file, because some earlier file in the group has probably already included them for you.

This means that when you add a new file to the build, and the unified sources get shuffled around, you end up with a long list of build breakages, some platform-specific, that you can only fix by repeating EWS trials. Here's an example: bugs.webkit.org/show_bug.cgi?id=189223. That patch added on new file in Source/WebCore/rendering, which required unrelated #include changes in at least:

rendering/RenderBlockFlow.cpp: rendering/RenderFrame.cpp: rendering/RenderImage.cpp:

How can we solve this? Should we have an EWS that builds non-unified? Can we somehow have the style checker do #include enforcement?

I think the solution to eagerly fix unified vs non-unified missing #includes would be strictly more costly than the problem — since the problem is that you sometimes have to fix unified vs non-unified missing #includes.

Geoff

# Don.Olmstead at sony.com (9 days ago)

This was actually something I wanted to bring up at the contributors meeting especially since I was bit by this recently with some CMake changes that busted WPE.

It would be nicer to move our tooling to clang. Include what you use is something I think would be of great use as well. CMake has built in support for a number of static checks blog.kitware.com/static-checks-with-cmake-cdash-iwyu-clang-tidy-lwyu-cpplint-and-cppcheck

My thoughts were to have a build-webkit –static-analysis flag that would do non-unified builds as well as potentially adding in dummy .cpp files for headers without an associated .cpp. It would run clang-tidy and include-what-you-use.

From: webkit-dev <webkit-dev-bounces at lists.webkit.org> On Behalf Of Darin Adler

Sent: Saturday, September 1, 2018 7:26 PM To: Simon Fraser <simon.fraser at apple.com>

Cc: WebKit Development <webkit-dev at lists.webkit.org>

Subject: Re: [webkit-dev] Unified sources have broken our #include hygiene

On Sep 1, 2018, at 5:14 PM, Simon Fraser <simon.fraser at apple.com<mailto:simon.fraser at apple.com>> wrote:

Unified sources allow you to get away without #including all the files you need in a .cpp file, because some earlier file in the group has probably already included them for you.

This means that when you add a new file to the build, and the unified sources get shuffled around, you end up with a long list of build breakages, some platform-specific, that you can only fix by repeating EWS trials.

Yes.

We knew this was going to happen when evaluated the proposal before we switched to unified sources. I believe you can find some discussion of it in webkit-dev.

How can we solve this?

I don’t think we should try to solve it.

To me at the moment, this seems to be a minor irritation. Even without unified sources, it’s common to get includes wrong for platforms other than the one you are testing on and find this out only when building on EWS.

I would be OK having an EWS server that builds various platforms without unified sources, but while it might help it might also hurt, adding more EWS results to interpret for each patch, and also finding theoretical problems that don’t affect any platform.

If there’s a tool that can figure out what files need to be included by analyzing source code, and that works well enough to be practical, I’d love to arrange it so that we can use that instead of having programmers have to write their own include statements. I’ve long been interested in include-what-you-use.org and wondered whether we could use it, but it sort of does the opposite, and the “multiple platforms” problem could well make even that tool impractical for us.

— Darin

# Keith Rollin (9 days ago)

On Sep 1, 2018, at 19:25, Darin Adler <darin at apple.com> wrote:

I’ve long been interested in include-what-you-use.org and wondered whether we could use it, but it sort of does the opposite, and the “multiple platforms” problem could well make even that tool impractical for us.

I’ve spent a couple of weeks-of-code looking into IWYU. It didn't seem practical for us. Our source code is not quite in the shape that IWYU desires and getting it into that shape would take a lot of effort (your comment about multiple platforms being an example of the issues I faced). And a trial of just running WTF through IWYU didn’t show any progress towards my goal at the time, which was to speed up builds by reducing the number of #includes. Keith Miller’s approach with the Unified Sources was way more effective. Establishing a goal of using IWYU to solve Simon’s problem is probably more trouble than it’s worth, IMO.

— Keith

# Frédéric Wang (12 hours ago)

I've recently found two non-trivial issues with unified builds. They are not missing #include but instead conflicts between C++ files:

  • Two C++ files including the same header but with a #define directive set to different values [1].
  • One C++ file including a header with implicit template instantiation in an inline function before an explicit template specialization in a second C++ file. [2]

I guess there could be more examples like this (e.g. functions with conflicting names in two C++ files...). Should we allow to prevent some files to be included in unified build? This is possible in Mozilla's build system for example [3].

Or should we always try to re-write the code to fix the conflicts between files? This might go against our usual practice: For example the current solution I have for [2] is to move the function from the header to its implementation C++ file, losing potential performance benefit of inlining...

[1] bugs.webkit.org/show_bug.cgi?id=189579 [2] bugs.webkit.org/show_bug.cgi?id=189541 [3] dxr.mozilla.org/mozilla-central/source/dom/canvas/moz.build#70

# Michael Catanzaro (8 hours ago)

On Thu, Sep 13, 2018 at 5:49 AM, Frédéric Wang <fwang at igalia.com>

wrote:

Should we allow to prevent some files to be included in unified build?

You can add @no-unify in the Sources.txt. This is usually easier than rewriting code.

# Maciej Stachowiak (6 hours ago)

On Sep 13, 2018, at 3:49 AM, Frédéric Wang <fwang at igalia.com> wrote:

Hi,

I've recently found two non-trivial issues with unified builds. They are not missing #include but instead conflicts between C++ files:

  • Two C++ files including the same header but with a #define directive set to different values [1].
  • One C++ file including a header with implicit template instantiation in an inline function before an explicit template specialization in a second C++ file. [2]

I guess there could be more examples like this (e.g. functions with conflicting names in two C++ files...). Should we allow to prevent some files to be included in unified build? This is possible in Mozilla's build system for example [3].

Or should we always try to re-write the code to fix the conflicts between files? This might go against our usual practice: For example the current solution I have for [2] is to move the function from the header to its implementation C++ file, losing potential performance benefit of inlining...

[1] bugs.webkit.org/show_bug.cgi?id=189579, bugs.webkit.org/show_bug.cgi?id=189579

^ Instead of your change here, I think the right fix is to make sure all files have the same value for IOSURFACE_CANVAS_BACKING_STORE. If headers depend on an #ifdef and different files have it set differently, that is a potential problem even without unified builds. Not in this case since the only difference is a static member function, but if the define affected data members, then it would be real bad for different files to have different values.

[2] bugs.webkit.org/show_bug.cgi?id=189541, bugs.webkit.org/show_bug.cgi?id=189541

I agree with the other comments that the template function should be defined in the header in the first place. It’s bad for different files to have different definitions of the same member function.

Sometimes it might be right to exclude files from unification, but in these two cases it seems like the unified build caught real problems with the code that should probably be fixed regardless.

Want more features?

Request early access to our private beta of readable email premium.