[webkit-dev] Deployment of new EWS Non-Unified builder

[Mark Lam]

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 <webkit-dev at lists.webkit.org> 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
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev