Skip to content
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

Closed
1 task done
jeffwidman opened this issue Dec 9, 2022 · 11 comments · Fixed by #9657 · May be fixed by #6290
Closed
1 task done

Stop hiding exception raised by composer #6289

jeffwidman opened this issue Dec 9, 2022 · 11 comments · Fixed by #9657 · May be fixed by #6290
Assignees
Labels
Batch How We Work: Feature. Outcome achieved within 1 iteration. Can live under an epic, or stand alone. EE Engineering Efficiency good first issue L: php:composer Issues and code for Composer T: tech-debt ⚙️

Comments

@jeffwidman
Copy link
Member

Is there an existing issue for this?

  • 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:

elsif error.message.include?("requirements could not be resolved")
# If there's no lockfile, there's no difference between running
# `composer install` and `composer update`, so we can easily check
# whether the existing requirements are resolvable for an install
check_original_requirements_resolvable unless lockfile
# If there *is* a lockfile we can't confidently distinguish between
# cases where we can't install and cases where we can't update. For
# now, we therefore just ignore the dependency.
nil

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 jeffwidman added L: php:composer Issues and code for Composer T: tech-debt ⚙️ labels Dec 9, 2022
jeffwidman added a commit that referenced this issue Dec 9, 2022
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
jeffwidman added a commit that referenced this issue Dec 9, 2022
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
@jeffwidman
Copy link
Member Author

jeffwidman commented Dec 9, 2022

Hmm... digging into this more:
This was originally added by #165

# We should raise a Dependabot::DependencyFileNotResolvable error
# here, but can't confidently distinguish between cases where we
# can't install and cases where we can't update. For now, we
# therefore just ignore the dependency.
nil

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 #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
Copy link
Member Author

@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...

jeffwidman added a commit that referenced this issue Dec 9, 2022
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
@stof
Copy link

stof commented Dec 12, 2022

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
Copy link
Member Author

jeffwidman commented Jan 13, 2023

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
Copy link

navarr commented Jan 13, 2023

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
Copy link
Member Author

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.

@navarr
Copy link

navarr commented Jan 13, 2023

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
Copy link
Member Author

Looks like this conversation has happened before:

@abdulapopoola abdulapopoola added the EE Engineering Efficiency label Mar 30, 2023
@abdulapopoola abdulapopoola added the Batch How We Work: Feature. Outcome achieved within 1 iteration. Can live under an epic, or stand alone. label Mar 14, 2024
@robaiken robaiken self-assigned this May 2, 2024
@robaiken robaiken linked a pull request May 2, 2024 that will close this issue
@robaiken
Copy link
Contributor

robaiken commented May 2, 2024

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
Copy link
Member Author

jeffwidman commented May 2, 2024

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.

@robaiken
Copy link
Contributor

robaiken commented May 3, 2024

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 #6289 (comment), 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Batch How We Work: Feature. Outcome achieved within 1 iteration. Can live under an epic, or stand alone. EE Engineering Efficiency good first issue L: php:composer Issues and code for Composer T: tech-debt ⚙️
Projects
Status: Done
6 participants
@navarr @stof @jeffwidman @abdulapopoola @robaiken and others