I think Ross’ point about supported ports being bitten by missing includes is
valid. I also agree that it can take more time (possibly a lot more time) for
an engineer stepping on the issue later in time to fix the missing includes vs
the original author fixing it if a non-unified EWS points out where the issue
is.
I have personally encountered this myself on Mac builds. The build error seemed
non-sensical at first as it had nothing to do with the patch I was developing.
This wasted some time for me to figure out that it wasn’t because I
accidentally made a bad edit somewhere (especially when my patch is necessarily
large). Once I realized that it was due to skew in the unification boundaries,
figuring out what include needed to be added also wasn’t obvious (at least at
the time where I encountered this years ago). I will grant that it appears to
not happen very often anymore (at least in my experience, I hasn’t happened to
me since), but there is an argument to be made that this may be thanks to
Italia’s effort of fixing them on a regular basis, and preventing it from
becoming a problem.
Anecdotal data aside, I would like to make a few points:
1. Build problems with skew in the unified files will only occur when one adds
new .cpp files into the unified mix. This means:
a. It will not affect most engineers who are just modifying code in
existing files.
b. It may affect engineers who are building features that require adding
new .cpp files (not .h files).
c. It may affect engineers who are refactoring code when it requires adding
new .cpp files.
d. It may affect engineers working on non-supported ports that are not
using unified ports.
The activities in (a) and (b) are common. (c) is more rare. (d), we agree we
shouldn’t care about (in the sense that it should not be a burden on engineers
of supported ports). As someone who does a lot of (b) and (c) activities, I
appreciate having this non-unified EWS.
Note that (a), (b), and (c) applies to supported ports working with unified
ports. This is not a port-specific issue, nor is just an unsupported port
issue.
2. Unlike build issues due platform specific code, build issues due to missing
includes tend to be easy to fix if identified by a non-unified EWS because:
a. The error will point exactly at the .cpp file that is failing to compile
which need the missing include (as opposed to a unified file with many .cpp
files).
b. The missing include is directly relevant to the patch being worked by
the engineer at the time. Hence, it is very likely the author of the patch
will immediately recognize the type / variable declaration that is missing, and
know which header file to include.
There is a rare chance that the non-unified build error involves some platform
specific code. In this case, I think it is fair to just ping the platform
maintainers on slack to give them a heads up about it, and ask them to help
resolve it. It does not need to block the patch from landing. Even so, this
makes it way easier for the platform maintainers to figure out the issue rather
than unsuspectingly stepping on it much later after hundreds of patches have
landed.
Some more thoughts in response to Darin below ...
> On Jun 4, 2022, at 5:42 AM, Darin Adler via webkit-dev
> <[email protected]> wrote:
>
> Yes, I don’t oppose adding another EWS bot, and I was not trying to argue
> against that proposal. I did intend to express my disagreement with many
> points made in follow-up replies that seem quite wrong to me.
>
> Three other thoughts:
>
> 1) Even though I don’t object to adding a new bot, I will say that the idea
> that a single “non-unified” bot will add a lot of value at very little cost
> to the WebKit project doesn’t sound right to me. These arguments about how
> long things will take don’t seem correct. My experience is that it’s quite
> difficult to find, understand, and resolve errors seen in bot builds, far
> more difficult than resolving errors I can see locally as I make code
> changes. In my view every EWS bot we add helps weigh down the project, making
> each change more difficult.
I agree that this is true in general. However, it the build issue is due to a
missing include, I think this is not true (see my point (2) above).
> 2) In my contributions, I don’t just want to add missing includes, I want to
> remove unnecessary ones taking full advantage of forward declarations and
> moving code out of headers. Too many includes and too much dependency has a
> dramatic bad effect on the project, making colossal project build times even
> worse.
I too try to do this when writing my patches. I would argue that a non-unified
EWS gives me greater confidence that a forward declaration is sufficient, and
that I’m not just mistakenly thinking that it is only because some other file
in the same unified unit also happen to include the header. So, again, a
non-unified EWS is helping here.
> 3) We had many, many problems with platform-specific include differences
> before we had unified builds, with frustrated contributors if they worked
> with any configuration that lacked an EWS bot. It may seem that the
> unified-build-specific problems are something fundamentally different, but
> this is not how I see it.
I disagree with the perspective that missing includes is not fundamentally
different than other port specific build issues (as I’ve described in my point
(2) above). Again, it is possible that the build issue is due to a port
specific issue. If it is port specific, the build error line will either be:
a. In a port specific file, or
b. In a port specific #if blob.
In that case, I think it is fair to not burden the engineer working on the
patch to resolved the issue. A courtesy heads up to the port maintainers on
slack will suffice.
As such, I also think that the non-unified EWS being green should not be a
blocker to landing a patch. But I think having it there for information will
help the situation. At minimum, even if every engineer simply ignores the
non-unified EWS, it also makes it easier for someone trying to fix a trim
missing include build issue to scan through PRs to look for this EWS failure in
order to narrow down on which patches (and therefore possible includes) to
focus on. Of course, this is assuming that our EWS visibility problem on PRs
in the current GitHub world gets fixed.
Mark
>
> — Darin
> _______________________________________________
> webkit-dev mailing list
> [email protected]
> https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-dev
