Unified sources have broken our #include hygiene
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: https://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
On Sep 1, 2018, at 17:14, Simon Fraser <simon.fraser@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: https://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!
Simon
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Sep 1, 2018, at 5:14 PM, Simon Fraser <simon.fraser@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
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 https://blog.kitware.com/static-checks-with-cmake-cdash-iwyu-clang-tidy-lwyu... 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@lists.webkit.org> On Behalf Of Darin Adler Sent: Saturday, September 1, 2018 7:26 PM To: Simon Fraser <simon.fraser@apple.com> Cc: WebKit Development <webkit-dev@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@apple.com<mailto:simon.fraser@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> 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
On Sep 1, 2018, at 19:25, Darin Adler <darin@apple.com> wrote:
I’ve long been interested in <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.
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
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: https://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
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] https://bugs.webkit.org/show_bug.cgi?id=189579 [2] https://bugs.webkit.org/show_bug.cgi?id=189541 [3] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/moz.build#70 On 02/09/2018 02:14, Simon Fraser 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: https://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
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
-- Frédéric Wang - frederic-wang.fr
On Sep 13, 2018, at 3:49 AM, Frédéric Wang <fwang@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] https://bugs.webkit.org/show_bug.cgi?id=189579 <https://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] https://bugs.webkit.org/show_bug.cgi?id=189541 <https://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.
[3] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/moz.build#70
On 02/09/2018 02:14, Simon Fraser 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: https://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
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
-- Frédéric Wang - frederic-wang.fr
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
participants (9)
-
Darin Adler
-
Don.Olmstead＠sony.com
-
Frédéric Wang
-
Geoffrey Garen
-
Keith Rollin
-
Maciej Stachowiak
-
Michael Catanzaro
-
Simon Fraser
-
Tim Horton