New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stop hiding exception raised by composer #6289
Comments
Expose the underlying composer error message when a dependency is not resolvable. 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. Fix #6289
Expose the underlying composer error message when a dependency is not resolvable. 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. Fix #6289
Hmm... digging into this more: dependabot-core/lib/dependabot/update_checkers/php/composer.rb Lines 132 to 136 in a65ad22
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 There are three test failures on #6290, and two are straightforward to fix but the third demonstrates exactly this problem:
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:
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 |
@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... |
Expose the underlying composer error message when a dependency is not resolvable. 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. Fix #6289
Platform packages are all prefixed with either As composer has |
Thanks. Agreed that asking I think the safe/helpful thing to do is simply leverage your suggested heuristics for when to swallow vs display the error message:
Let me try that. |
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? |
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 #6290 to at least expose errors when non-platform-related. |
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, |
Looks like this conversation has happened before: |
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. |
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 #6289 (comment), 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. |
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.
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. |
Is there an existing issue for this?
Code improvement description
While debugging a customer issue, we ran into the problem that the error output from the
composer
helper was being swallowed:dependabot-core/composer/lib/dependabot/composer/update_checker/version_resolver.rb
Lines 295 to 304 in b911f9d
The underlying error:
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.
The text was updated successfully, but these errors were encountered: