Improve detection of unsupported cargo toolchains #8181

deivid-rodriguez · 2023-10-12T11:07:11Z

We're getting the following flaky in cargo:

https://github.com/dependabot/dependabot-core/actions/runs/6493735738/job/17635263354

Failures:

  1) Dependabot::Cargo::UpdateChecker::VersionResolver latest_resolvable_version when using a toolchain that is too old raises a helpful error
     Failure/Error:
       expect { subject }
         .to raise_error(Dependabot::DependencyFileNotEvaluatable)

       expected Dependabot::DependencyFileNotEvaluatable, got #<Dependabot::SharedHelpers::HelperSubprocessFailed: Blocking waiting for file lock on package cache
       error: Unable to update registry `crates-io`

       Caused by:
         usage of sparse registries requires `-Z sparse-registry`> with backtrace:
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:153:in `run_cargo_command'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:137:in `run_cargo_update_command'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:307:in `block (2 levels) in original_requirements_resolvable?'
         # /home/dependabot/common/lib/dependabot/shared_helpers.rb:195:in `with_git_configured'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:306:in `block in original_requirements_resolvable?'
         # /home/dependabot/common/lib/dependabot/shared_helpers.rb:56:in `block in in_a_temporary_directory'
         # /home/dependabot/common/lib/dependabot/shared_helpers.rb:56:in `chdir'
         # /home/dependabot/common/lib/dependabot/shared_helpers.rb:56:in `in_a_temporary_directory'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:303:in `original_requirements_resolvable?'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:298:in `resolvability_error?'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:245:in `handle_cargo_errors'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:59:in `rescue in fetch_latest_resolvable_version'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:41:in `fetch_latest_resolvable_version'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:33:in `latest_resolvable_version'
         # ./spec/dependabot/cargo/update_checker/version_resolver_spec.rb:71:in `block (3 levels) in <top (required)>'
         # ./spec/dependabot/cargo/update_checker/version_resolver_spec.rb:177:in `block (5 levels) in <top (required)>'
         # ./spec/dependabot/cargo/update_checker/version_resolver_spec.rb:177:in `block (4 levels) in <top (required)>'
         # /home/dependabot/common/spec/spec_helper.rb:46:in `block (2 levels) in <top (required)>'
     # ./spec/dependabot/cargo/update_checker/version_resolver_spec.rb:177:in `block (4 levels) in <top (required)>'
     # /home/dependabot/common/spec/spec_helper.rb:46:in `block (2 levels) in <top (required)>'

Finished in 33.38 seconds (files took 1.24 seconds to load)
360 examples, 1 failure

Failed examples:

rspec ./spec/dependabot/cargo/update_checker/version_resolver_spec.rb:176 # Dependabot::Cargo::UpdateChecker::VersionResolver latest_resolvable_version when using a toolchain that is too old raises a helpful error

It seems confusing to me because we fail to categorize the error as matching usage of sparse registries requires `-Z sparse-registry` which is what determines that the cargo version is too old. But still, the command being raised is including that.

The problem is that we fail to match the string in the original error, but then when later trying to figure out whether the error was a resolvability issue, we end up running another cargo command to see if the original requirements were resolvable, and in that case, the usage of sparse registries requires `-Z sparse-registry` string is in the output.

So I'm wondering why the original error did not include the string.

To be able to verify this, we could return "nil" from original_requirements_resolvable if this happens, and let the original error be raised in that case.

deivid-rodriguez · 2023-10-12T11:52:18Z

That did the trick and I can see the real culprit now! Original error is:

  1) Dependabot::Cargo::UpdateChecker::VersionResolver latest_resolvable_version when using a toolchain that is too old raises a helpful error
     Failure/Error:
       expect { subject }
         .to raise_error(Dependabot::DependencyFileNotEvaluatable)

       expected Dependabot::DependencyFileNotEvaluatable, got #<Dependabot::SharedHelpers::HelperSubprocessFailed: info: syncing channel updates for '1.67-x86_64-unknown-linux-gnu'
       info: latest update on 2023-02-09, rust version 1.67.1 (d5a82bbd2 2023-02-07)
       info: downloading component 'rustc'
       info: downloading component 'cargo'
       info: downloading component 'rust-std'
       info: downloading component 'rust-docs'
       info: downloading component 'rustfmt'
       info: downloading component 'clippy'
       info: removing previous version of component 'rustc'
       warning: during uninstall...on of component 'rust-docs'
       warning: during uninstall component rust-docs was not found
       info: removing previous version of component 'rustfmt'
       warning: during uninstall component rustfmt was not found
       info: removing previous version of component 'clippy'
       warning: during uninstall component clippy was not found
       info: installing component 'rustc'
       info: rolling back changes
       error: could not read downloaded file: '/opt/rust/downloads/e27ec0c6d1a2b2b38e5258904c3741ddb246bff5715aa95e595f818aa77f7bee'> with backtrace:
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:153:in `run_cargo_command'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:137:in `run_cargo_update_command'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:47:in `block (2 levels) in fetch_latest_resolvable_version'
         # /home/dependabot/common/lib/dependabot/shared_helpers.rb:195:in `with_git_configured'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:46:in `block in fetch_latest_resolvable_version'
         # /home/dependabot/common/lib/dependabot/shared_helpers.rb:56:in `block in in_a_temporary_directory'
         # /home/dependabot/common/lib/dependabot/shared_helpers.rb:56:in `chdir'
         # /home/dependabot/common/lib/dependabot/shared_helpers.rb:56:in `in_a_temporary_directory'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:43:in `fetch_latest_resolvable_version'
         # ./lib/dependabot/cargo/update_checker/version_resolver.rb:33:in `latest_resolvable_version'
         # ./spec/dependabot/cargo/update_checker/version_resolver_spec.rb:71:in `block (3 levels) in <top (required)>'
         # ./spec/dependabot/cargo/update_checker/version_resolver_spec.rb:177:in `block (5 levels) in <top (required)>'
         # ./spec/dependabot/cargo/update_checker/version_resolver_spec.rb:177:in `block (4 levels) in <top (required)>'
         # /home/dependabot/common/spec/spec_helper.rb:46:in `block (2 levels) in <top (required)>'
     # ./spec/dependabot/cargo/update_checker/version_resolver_spec.rb:177:in `block (4 levels) in <top (required)>'
     # /home/dependabot/common/spec/spec_helper.rb:46:in `block (2 levels) in <top (required)>'

deivid-rodriguez · 2023-10-12T13:01:36Z

I ended up with three different commits here, but I'm not sure if this is correct:

First commit exposes the original error as explained in this PR body. This one seems ok.
Second commit detects an old Rust version in the output as another way of detecting old Rust. The rationale is that I believe if the automatic download/install of old Rust fails, we don't even get the chance to use it, and can't detect that it does not support the sparse flag.
Third commit removes this detection from lockfile updater. I believe it's enough to do this in update checker since we always go through that before trying to update lockfiles.

deivid-rodriguez · 2023-10-12T13:07:13Z

By the way, this flakes seems to be happening consistently right now? At least I'm not being able to get #8174 green by retrying!

jakecoffman · 2023-10-12T13:49:37Z

cargo/spec/dependabot/cargo/file_updater/lockfile_updater_spec.rb

-          .to raise_error(Dependabot::DependencyFileNotEvaluatable)
-      end
-    end
-


Can the test go back in now that it works?

This is me completely removing the detection from lockfile updater, because I believe this should never be triggered. We always go through UpdateChecker first, and we would detect it there before?

If this is really needed, then I need to duplicate the fix in the updater to get the test to pass.

Maybe move it to the UpdateChecker specs?

It's already there!

Oh ok, then we're good to go!

Actually my assumption was incorrect. I think in the case of security updates, we won't go through VersionResolver sometimes but pick the lowest fixed version directly from LatestVersionFinder. So the check in LockfileUpdater may be still needed. I'll port the fix and restore the test just in case.

I duplicated the fix in LockfileUpdater at 39bb742, and restored the spec 👍.

jakecoffman · 2023-10-12T13:54:26Z

cargo/lib/dependabot/cargo/update_checker/version_resolver.rb

+            false
+          elsif original_requirements_resolvable == false
+            true
+          end


It's still early here, trying to understand this change. It looks the same as the code it replaces?

The previous code returns always either true or false. The new code returns either true, false, or nil if we couldn't find a definitive hint about a resolvability issue.

Satisfying RuboCop requests made it quite unreadable, I'll add a comment.

Not a big deal, I was trying to figure out the significance of the change.

Have a look at e1c8dce, it should be easier to read!

jakecoffman

Looks good, would be good to have a test to make sure this doesn't break in the future. I recall it was causing a lot of unknown errors at the time.

Edit: @deivid-rodriguez says there's already a test there so we good!

This is almost the same thing as before, but `raise` without an argument lets the original error instance be raised. However, `raise e` raises a new instance of the same error, resulting in slightly harder to parse backtraces.

Currently we look for an error when trying to use a flag that's not supported by old toolchains. However, I think cargo downloads the user selected toolchain on the fly and that may fail before even getting the error about the unsupported flag. I think we may also detect that the old version is getting downloaded and error in the same way.

deivid-rodriguez requested a review from a team as a code owner October 12, 2023 11:07

github-actions bot added the L: rust:cargo Rust crates via cargo label Oct 12, 2023

deivid-rodriguez force-pushed the deivid-rodriguez/cargo-ci branch 2 times, most recently from cda4776 to f51f2a0 Compare October 12, 2023 11:15

deivid-rodriguez requested a review from jakecoffman October 12, 2023 13:01

jakecoffman reviewed Oct 12, 2023

View reviewed changes

jakecoffman approved these changes Oct 12, 2023

View reviewed changes

deivid-rodriguez added 2 commits October 12, 2023 16:09

Prefer raise to raise e

9bcb67b

This is almost the same thing as before, but `raise` without an argument lets the original error instance be raised. However, `raise e` raises a new instance of the same error, resulting in slightly harder to parse backtraces.

If we're not sure it was a resolvability error, raise original error

e1c8dce

deivid-rodriguez force-pushed the deivid-rodriguez/cargo-ci branch 2 times, most recently from e472404 to 579fa9c Compare October 12, 2023 14:20

deivid-rodriguez force-pushed the deivid-rodriguez/cargo-ci branch from 579fa9c to 39bb742 Compare October 12, 2023 14:24

deivid-rodriguez changed the title ~~Troubleshoot cargo CI flaky failures~~ Improve detection of unsupported cargo toolchains Oct 12, 2023

deivid-rodriguez merged commit 370fe77 into main Oct 12, 2023
97 checks passed

deivid-rodriguez deleted the deivid-rodriguez/cargo-ci branch October 12, 2023 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve detection of unsupported cargo toolchains #8181

Improve detection of unsupported cargo toolchains #8181

deivid-rodriguez commented Oct 12, 2023

deivid-rodriguez commented Oct 12, 2023 •

edited

Loading

deivid-rodriguez commented Oct 12, 2023

deivid-rodriguez commented Oct 12, 2023 •

edited

Loading

jakecoffman Oct 12, 2023

deivid-rodriguez Oct 12, 2023

jakecoffman Oct 12, 2023 •

edited

Loading

deivid-rodriguez Oct 12, 2023

jakecoffman Oct 12, 2023

deivid-rodriguez Oct 12, 2023

deivid-rodriguez Oct 12, 2023

jakecoffman Oct 12, 2023

deivid-rodriguez Oct 12, 2023

deivid-rodriguez Oct 12, 2023

jakecoffman Oct 12, 2023

deivid-rodriguez Oct 12, 2023

jakecoffman left a comment •

edited

Loading

Improve detection of unsupported cargo toolchains #8181

Improve detection of unsupported cargo toolchains #8181

Conversation

deivid-rodriguez commented Oct 12, 2023

deivid-rodriguez commented Oct 12, 2023 • edited Loading

deivid-rodriguez commented Oct 12, 2023

deivid-rodriguez commented Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakecoffman Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakecoffman left a comment • edited Loading

Choose a reason for hiding this comment

deivid-rodriguez commented Oct 12, 2023 •

edited

Loading

deivid-rodriguez commented Oct 12, 2023 •

edited

Loading

jakecoffman Oct 12, 2023 •

edited

Loading

jakecoffman left a comment •

edited

Loading