Four Broken Builds

In the last month, I’ve done four PRs, fixing four broken builds in major PHP libraries.  Here is what went wrong, how I fixed them, and what I learnt.

Pimple

First up, the Pimple project.  Pimple doesn’t install PHPUnit through Composer – it uses the PHP installation that comes with a Travis CI container.  Travis had bumped to PHPUnit 6.0, and Pimple’s tests only work in PHPUnit <=5.7.

Changing the tests wasn’t going to help, because Pimple still supports PHP 5.3 [sic].  Making the tests compatible with PHPUnit 6 (requiring PHP 5.6+) wasn’t going to fix the build.

Fabien Potencier’s concerns about installing test tools through Composer are that the PHP OSS ecosystem starts chasing its tail if dev-master of PHPUnit requires a stable Symfony component, and the component require-devs a stable PHPUnit release.  It all gets a bit ‘Yo dawg’, and it is, apparently, easier to treat PHPUnit as an external binary.  I’m not sure I’ve fully understood how using a PHAR helps, but still, project maintainers are always going to have constraints on how they will allow a build to be fixed.

In the end, the only way to get the build back to green and keep the lead maintainer happy all at the same time was to use the Symfony PHPUnit bridge, designed to solve this problem.  In my Pull Request I made it clear that the Travis change had initiated the build break, and bore in mind what I’d learnt about the maintainer’s preferred approach to this kind of thing.

Results for Pimple

PR: Merged    Build: Green

Monolog

Monolog were facing two challenges with their build when I came across it.  The first was an obscure PHP DateTime bug had turned up in 7.1.3, having been absent from the previous version.  When Travis upgraded to the latest PHP patch release, Monolog PRs all started having red builds and confused authors.

The solution in this case was to skip the test that was failing, where the iffy PHP version was detected.  I was particularly lucky here to have chosen that exact moment to upgrade my laptop from PHP 7.1.0 ⇒ 7.1.3 while trying to reproduce this bug.

if (PHP_VERSION_ID === 70103) {
    $this->markTestSkipped();
}

The second issue was that Composer was segfaulting on Travis CI’s ‘php nightly’ build, using a recently compiled version of PHP internals’ master branch.

For this, all I did was moved the offending build in the .travis.yml file as follows:

allow_failures:
    - php: nightly

I haven’t heard back from Jordi on this PR yet.  Perhaps he plans to wait until the PHP nightly build gets its act together and stops segfaulting, and 7.1.4 stable comes out with working DateTime objects.  Time will tell how long this build will remain red.

Results for Monolog

PR: Merged    Build: Green

Doctrine

Doctrine’s build was a bugger to fathom.  What was going wrong on the Doctrine front was caused by a bug in the Symfony Yaml component.  It took me more tinkering with Doctrine’s composer.yml than I’d care to admit to, but that was the conclusion.

Specifically, Doctrine uses "minimum-stability": "dev" in its own Composer file.  The buggy commit in symfony/yaml didn’t even have a patch release tag above it.  I must admit I was a little surprised by this, but the reasoning was made clearer when:

  1. I realised that the end user’s application preferences on stability would take precedence over Doctrine regarding what Doctrine dependency versions to install.  This means the setting only really affects those developing Doctrine or bouncing around in its build process; and
  2. this system of using dev-master for testing had allowed Ocramius’s friend @lcobucci on Github to report the issue introduced into the Symfony Yaml parser.  They were already fixing it when I first realised what the problem was.

@Sam-Burns if we didn’t keep dev dependencies, this sort of issue would never be discovered in first place, [causing] more issues downstream – @Ocramius

When I sent the PR to Doctrine, suggesting pinning to a stable, tagged release of this dev dependency, to make the build more reliable and everyone’s Pull Requests turn green, Ocramius ended up choosing not to accept it.  I can kind of see his point, with the early warning of upstream bugs, but I still think in his shoes I might have got my own build green first, then worried about helping other projects.  A noble sacrifice.

I have since had two PRs to Doctrine accepted: introducing a prefer-lowest build, and bumping to PHPUnit 6.

Results for Doctrine

PR: Closed    Build: Red, turning Green later that weekend

Prophecy

The build had broken because, presumably in some sort of industrial sabotage, a PHPUnit maintainer had done something to Prophecy’s dependencies that had broken the build in a PR, which then got merged by a Prophecy core maintainer. Of the four build breaks of my month of fixing them, this was the only one initiated by human error directly at the point of breaking.

I sorted out the dependency graph in a way that it retrieved the dependencies it was supposed to, and added a minimum dependency build to Prophecy, to stop them from having the same issue again.

Results for Prophecy

PR: Merged    Build: Green

What I Learnt

One of the builds was broken because a maintainer accidentally merged a broken PR.  The other three were broken because of changes elsewhere: a PHP bug, a Symfony bug, and an update to Travis.

A total of two of the build breaks could have been avoided by a more conventional approach to dependency management and Composer.  Overall, putting your dependencies in the composer.json, and coupling the code as loosely as possible to the platform, seem to avoid unexpected issues – although there are occasionally reasons not to.

I learnt that studying project maintainers’ architectural preferences can help a PR be waived through swiftly.  Do your homework and work out what maintainers will want, before wading in with the One True Way, and you’ll get a better response.  If the architecture is weird, it’s because someone wanted it like that, so compromise is key to getting a PR through review.  Things move fastest when this compromise starts before the PR is created and discussion begins.