Stop hiding exception raised by composer

[jeffwidman]

Is there an existing issue for this?

• [X] I have searched the existing issues

Code improvement description

While debugging a customer issue, we ran into the problem that the error output from the composer helper was being swallowed: https://github.com/dependabot/dependabot-core/blob/b911f9dfe1e11d19edcafb11e83d305758c97f85/composer/lib/dependabot/composer/update_checker/version_resolver.rb#L295-L304

The underlying error:

Your requirements could not be resolved to an installable set of packages. (Dependabot::SharedHelpers::HelperSubprocessFailed)
  Problem 1
    - php-amqplib/php-amqplib v2.11.3 conflicts with php[7.4.0].
    - php-amqplib/php-amqplib v2.11.3 conflicts with php[7.4.0].
    - Installation request for php 7.4.0 -> satisfiable by php[7.4.0].
    - Installation request for php-amqplib/php-amqplib (locked at v2.11.3, required as ^2.6) -> satisfiable by php-amqplib/php-amqplib[v2.11.3].

Swallowing the error makes it hard for users to figure what's going on.

The original motivation for swallowing was in the early days of Dependabot Preview, it opened issues for exceptions raised in the updater/core which has forced us to suppress certain errors that might be spammy or intermittent, e.g. resolvability errors. In version updates we no longer create issues so attaching errors to the last update job means we can attach any number of errors to highlight potential issues without spamming users.

A potential improvement would be to expose structured errors per checked dependency to make it clearer which dependency caused the error.

[jeffwidman]

Hmm... digging into this more: This was originally added by https://github.com/dependabot/dependabot-core/pull/165/ https://github.com/dependabot/dependabot-core/blob/a65ad2269efdc597ad01142ccc7a7e4664039f92/lib/dependabot/update_checkers/php/composer.rb#L132-L136

So the exception in https://github.com/dependabot/dependabot-core/pull/6290/files#diff-82bee76077e5dbd4f3073948aa2ad29eec23333f9b4e06550fbf54c259875a24R304 is the right one to raise... but the problem here is that PHP has native extensions that we don't install in the place where we're running dependabot-core... so a resolvability error may be because of missing PHP extensions, not because of a normal resolvability error... so we can't just blindly raise the exception w/o breaking everyone using native PHP extensions.

There are three test failures on https://github.com/dependabot/dependabot-core/pull/6290, and two are straightforward to fix but the third demonstrates exactly this problem:

 1) Dependabot::Composer::UpdateChecker#latest_resolvable_version with an update that can't resolve
     Failure/Error: raise Dependabot::DependencyFileNotResolvable, sanitized_message

     Dependabot::DependencyFileNotResolvable:
       Your requirements could not be resolved to an installable set of packages.
         Problem 1
           - Root composer.json requires longman/telegram-bot >= 2.1.5, <= 0.53.0, found longman/telegram-bot[dev-feature/database-migrations, dev-feature/refactor-app-design, dev-keyboard-improvement, dev-custom-api-uri, dev-docker, dev-master, dev-develop, 0.16.0, ..., 0.80.0] but it does not match the constraint.
         Problem 2
           - phalcon/devtools is locked to version v3.2.9 and an update of this package was not requested.
           - phalcon/devtools v3.2.9 requires ext-phalcon ~3.1 -> it is missing from your system. Install or enable PHP's phalcon extension.
...

We want to be able to report problem 1 to the user, but swallow problem 2... but I'm uncertain the best way to identify when it's a missing extension problem:

1. ext-* 👈 are all extensions prefixed with ext-?
2. "it is missing from your system. Install or enable PHP's * extension." 👈 we could probably grep for this string

Additionally, I think Composer just gives the errors to us as an unstructured text blob, so we'd have to simply swallow the entire error if it requires any missing extension problem. Ideally we could request composer to return the errors via JSON so that we could parse which to swallow and which to expose... that would also let composer tell us the error type (missing install vs non-resolvable).

[jeffwidman]

@driskell @markdorison @stof I'd appreciate any advice re: ☝️ as I'd like to be able to expose resolvability errors to Dependabot users, but not sure how to handle this "missing native extension" case... as we aren't going to pre-install all native PHP extensions within the Dependabot dockerfile...

[stof]

Platform packages are all prefixed with either php-, ext- or lib- (except for the php package itself) and they have no / in their name (while userland packages always have one).

As composer has --ignore-platform-req=... to ignore a particular platform requirement (and --ignore-platform-reqs to ignore them all). So in case resolution fails with an error involving a platform package, you could try resolving again ignoring that one. Another option is to expose a configuration setting to let projects configure the composer platform config setting when running dependabot, which might be a much safer option than ignoring platform requirements as they could configure dependabot to run with a platform matching their prod platform.

[jeffwidman]

Thanks.

Agreed that asking composer to ignore platform requirements when resolving feels dangerous... most of the time it'll work fine, but when it doesn't... I'd rather we fail to open a valid PR than we open an invalid PR that could cause breakage in prod.

I think the safe/helpful thing to do is simply leverage your suggested heuristics for when to swallow vs display the error message:

Platform packages are all prefixed with either php-, ext- or lib- (except for the php package itself) and they have no / in their name (while userland packages always have one).

Let me try that.

[navarr]

Composer platform dependencies is definitely not obvious when working with a tool like Dependabot.

Specifying what extensions/php version are used is available: https://getcomposer.org/doc/06-config.md#platform.

Sometimes this isn't desirable to set in the composer.json file; so perhaps it could be a dependabot yml config?

[jeffwidman]

Oh I see what you guys are getting at... 👍

At this time, we don't offer custom platforms for running Dependabot (at least the native GitHub version), but you're right, we could potentially let users say "I'm aware of the risks and would prefer the convenience of ignoring this plaform dep when resolving updates"...

TBH, we're pretty hesitant to add configs because then we have to support them forever, but I could see this use case happening across pretty much all ecosystems (although not all package managers support ignoring platform-specific deps) so we may eventually add it... but it'd be a larger lift.

For now I'll try to make baby-steps on https://github.com/dependabot/dependabot-core/pull/6290 to at least expose errors when non-platform-related.

[navarr]

This config option is less ignoring platform-specific dependencies, and more telling composer what platform it should be installing dependencies for (even if it doesn't match the platform running the update).

Essentially, composer config platform '{"php":"8.1", "ext-intl": "4"}'

[jeffwidman]

Looks like this conversation has happened before:

• https://github.com/dependabot/dependabot-core/pull/890

[robaiken]

Going forward, we will expose any errors that occur and proceed with installing the most widely used PHP extensions. Please let me know if you have any feedback or thoughts on this approach.

[jeffwidman]

I think it's a great idea to install a few more of the widely used PHP extensions... I assume you mean by pre-installing into the Docker container?

But I think it's unrealistic to install all the ones users will want. There will be a long tail where maybe 2-3 users want a particular extension, so the list will get quite long.

Additionally, these aren't "install and forget" because since they're native extensions, as we slowly upgrade Ubuntu versions it can make it hard to upgrade... I've hit this in the past where a native extension required some other package, but than that package changed names under the next version of Ubuntu and untangling all that to understand whether we actually still needed to block on a particular package or not took a while.

So it makes sense that we only install the more popular ones. 👍 for that.

However, that means that we'll still hit the problem of https://github.com/dependabot/dependabot-core/issues/6289#issuecomment-1344755822, only less frequently. But we'll still hit it.

So that means we'll probably want to "resolve" this by logging the error rather than outright raising the exception. Because if we raise, we completely kill the job, vs if we log and continue, we at least give the user a partial update of their other dependencies.

[robaiken]

I think logging the error instead of raising it is a good call. I'll update my PR to do that. The main thing I want is to understand why Dependabot fails, not just hide the problem. Plus, users should be able to see why Dependabot couldn't run when extensions are missing.

However, that means that we'll still hit the problem of https://github.com/dependabot/dependabot-core/issues/6289#issuecomment-1344755822, only less frequently. But we'll still hit it.

I'm okay with that as long as we let the user know what went wrong and we can see why it failed too. For now, I am focusing on getting this to work with the most popular extensions. If this approach works for 80% of the most popular extensions, I think we have made an improvement. Then we can reevaluate later.