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.

Sign up for GitHub

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

Jump to bottom

Fix some unnecessary top level dependency downgrades #6535

Merged
merged 2 commits into from
Mar 24, 2023
Merged

Fix some unnecessary top level dependency downgrades #6535

merged 2 commits into from
Mar 24, 2023

Conversation

deivid-rodriguez
Copy link
Member

What was the end-user or developer problem that led to this PR?

With the following Gemfile & Gemfile.lock files:

source "https://rubygems.org"

gem "govuk_app_config"
gem "activesupport", "7.0.4.3"
GEM
  remote: https://rubygems.org/
  specs:
    actionpack (7.0.4.1)
      actionview (= 7.0.4.1)
      activesupport (= 7.0.4.1)
      rack (~> 2.0, >= 2.2.0)
      rack-test (>= 0.6.3)
      rails-dom-testing (~> 2.0)
      rails-html-sanitizer (~> 1.0, >= 1.2.0)
    actionview (7.0.4.1)
      activesupport (= 7.0.4.1)
      builder (~> 3.1)
      erubi (~> 1.4)
      rails-dom-testing (~> 2.0)
      rails-html-sanitizer (~> 1.1, >= 1.2.0)
    activesupport (7.0.4.1)
      concurrent-ruby (~> 1.0, >= 1.0.2)
      i18n (>= 1.6, < 2)
      minitest (>= 5.1)
      tzinfo (~> 2.0)
    builder (3.2.4)
    concurrent-ruby (1.2.2)
    crass (1.0.6)
    erubi (1.12.0)
    govuk_app_config (4.13.0)
      logstasher (~> 2.1)
      plek (>= 4, < 6)
      prometheus_exporter (~> 2.0)
      puma (>= 5.6, < 7.0)
      rack-proxy (~> 0.7)
      sentry-rails (~> 5.3)
      sentry-ruby (~> 5.3)
      statsd-ruby (~> 1.5)
      unicorn (~> 6.1)
    i18n (1.12.0)
      concurrent-ruby (~> 1.0)
    kgio (2.11.4)
    logstasher (2.1.5)
      activesupport (>= 5.2)
      request_store
    loofah (2.19.1)
      crass (~> 1.0.2)
      nokogiri (>= 1.5.9)
    method_source (1.0.0)
    mini_portile2 (2.8.1)
    minitest (5.18.0)
    nio4r (2.5.8)
    nokogiri (1.14.0)
      mini_portile2 (~> 2.8.0)
      racc (~> 1.4)
    nokogiri (1.14.0-aarch64-linux)
      racc (~> 1.4)
    nokogiri (1.14.0-arm64-darwin)
      racc (~> 1.4)
    nokogiri (1.14.0-x86_64-linux)
      racc (~> 1.4)
    plek (4.1.0)
    prometheus_exporter (2.0.3)
      webrick
    puma (6.0.0)
      nio4r (~> 2.0)
    racc (1.6.2)
    rack (2.2.6.3)
    rack-proxy (0.7.4)
      rack
    rack-test (2.0.2)
      rack (>= 1.3)
    rails-dom-testing (2.0.3)
      activesupport (>= 4.2.0)
      nokogiri (>= 1.6)
    rails-html-sanitizer (1.5.0)
      loofah (~> 2.19, >= 2.19.1)
    railties (7.0.4.1)
      actionpack (= 7.0.4.1)
      activesupport (= 7.0.4.1)
      method_source
      rake (>= 12.2)
      thor (~> 1.0)
      zeitwerk (~> 2.5)
    raindrops (0.20.0)
    rake (13.0.6)
    request_store (1.5.1)
      rack (>= 1.4)
    sentry-rails (5.5.0)
      railties (>= 5.0)
      sentry-ruby (~> 5.5.0)
    sentry-ruby (5.5.0)
      concurrent-ruby (~> 1.0, >= 1.0.2)
    statsd-ruby (1.5.0)
    thor (1.2.1)
    tzinfo (2.0.6)
      concurrent-ruby (~> 1.0)
    unicorn (6.1.0)
      kgio (~> 2.6)
      raindrops (~> 0.7)
    webrick (1.7.0)
    zeitwerk (2.6.3)

PLATFORMS
  aarch64-linux
  arm64-darwin
  ruby
  x86_64-linux

DEPENDENCIES
  govuk_app_config

BUNDLED WITH

Running bundle lock results in the following Gemfile.lock diff:

 GEM
   remote: https://rubygems.org/
   specs:
-    actionpack (7.0.4.1)
-      actionview (= 7.0.4.1)
-      activesupport (= 7.0.4.1)
-      rack (~> 2.0, >= 2.2.0)
-      rack-test (>= 0.6.3)
-      rails-dom-testing (~> 2.0)
-      rails-html-sanitizer (~> 1.0, >= 1.2.0)
-    actionview (7.0.4.1)
-      activesupport (= 7.0.4.1)
-      builder (~> 3.1)
-      erubi (~> 1.4)
-      rails-dom-testing (~> 2.0)
-      rails-html-sanitizer (~> 1.1, >= 1.2.0)
-    activesupport (7.0.4.1)
+    activesupport (7.0.4.3)
       concurrent-ruby (~> 1.0, >= 1.0.2)
       i18n (>= 1.6, < 2)
       minitest (>= 5.1)
       tzinfo (~> 2.0)
-    builder (3.2.4)
     concurrent-ruby (1.2.2)
-    crass (1.0.6)
-    erubi (1.12.0)
-    govuk_app_config (4.13.0)
-      logstasher (~> 2.1)
-      plek (>= 4, < 6)
-      prometheus_exporter (~> 2.0)
-      puma (>= 5.6, < 7.0)
-      rack-proxy (~> 0.7)
-      sentry-rails (~> 5.3)
-      sentry-ruby (~> 5.3)
-      statsd-ruby (~> 1.5)
-      unicorn (~> 6.1)
+    govuk_app_config (0.1.0)
     i18n (1.12.0)
       concurrent-ruby (~> 1.0)
-    kgio (2.11.4)
-    logstasher (2.1.5)
-      activesupport (>= 5.2)
-      request_store
-    loofah (2.19.1)
-      crass (~> 1.0.2)
-      nokogiri (>= 1.5.9)
-    method_source (1.0.0)
-    mini_portile2 (2.8.1)
     minitest (5.18.0)
-    nio4r (2.5.8)
-    nokogiri (1.14.0)
-      mini_portile2 (~> 2.8.0)
-      racc (~> 1.4)
-    nokogiri (1.14.0-aarch64-linux)
-      racc (~> 1.4)
-    nokogiri (1.14.0-arm64-darwin)
-      racc (~> 1.4)
-    nokogiri (1.14.0-x86_64-linux)
-      racc (~> 1.4)
-    plek (4.1.0)
-    prometheus_exporter (2.0.3)
-      webrick
-    puma (6.0.0)
-      nio4r (~> 2.0)
-    racc (1.6.2)
-    rack (2.2.6.3)
-    rack-proxy (0.7.4)
-      rack
-    rack-test (2.0.2)
-      rack (>= 1.3)
-    rails-dom-testing (2.0.3)
-      activesupport (>= 4.2.0)
-      nokogiri (>= 1.6)
-    rails-html-sanitizer (1.5.0)
-      loofah (~> 2.19, >= 2.19.1)
-    railties (7.0.4.1)
-      actionpack (= 7.0.4.1)
-      activesupport (= 7.0.4.1)
-      method_source
-      rake (>= 12.2)
-      thor (~> 1.0)
-      zeitwerk (~> 2.5)
-    raindrops (0.20.0)
-    rake (13.0.6)
-    request_store (1.5.1)
-      rack (>= 1.4)
-    sentry-rails (5.5.0)
-      railties (>= 5.0)
-      sentry-ruby (~> 5.5.0)
-    sentry-ruby (5.5.0)
-      concurrent-ruby (~> 1.0, >= 1.0.2)
-    statsd-ruby (1.5.0)
-    thor (1.2.1)
     tzinfo (2.0.6)
       concurrent-ruby (~> 1.0)
-    unicorn (6.1.0)
-      kgio (~> 2.6)
-      raindrops (~> 0.7)
-    webrick (1.7.0)
-    zeitwerk (2.6.3)
 
 PLATFORMS
   aarch64-linux
@@ -104,6 +21,7 @@ PLATFORMS
   x86_64-linux
 
 DEPENDENCIES
+  activesupport (= 7.0.4.3)
   govuk_app_config
 
 BUNDLED WITH

which is downgrading the top level dependency govuk_app_config.

Instead, it should result in the following diff:

 GEM
   remote: https://rubygems.org/
   specs:
-    actionpack (7.0.4.1)
-      actionview (= 7.0.4.1)
-      activesupport (= 7.0.4.1)
+    actionpack (7.0.4.3)
+      actionview (= 7.0.4.3)
+      activesupport (= 7.0.4.3)
       rack (~> 2.0, >= 2.2.0)
       rack-test (>= 0.6.3)
       rails-dom-testing (~> 2.0)
       rails-html-sanitizer (~> 1.0, >= 1.2.0)
-    actionview (7.0.4.1)
-      activesupport (= 7.0.4.1)
+    actionview (7.0.4.3)
+      activesupport (= 7.0.4.3)
       builder (~> 3.1)
       erubi (~> 1.4)
       rails-dom-testing (~> 2.0)
       rails-html-sanitizer (~> 1.1, >= 1.2.0)
-    activesupport (7.0.4.1)
+    activesupport (7.0.4.3)
       concurrent-ruby (~> 1.0, >= 1.0.2)
       i18n (>= 1.6, < 2)
       minitest (>= 5.1)
@@ -71,9 +71,9 @@ GEM
       nokogiri (>= 1.6)
     rails-html-sanitizer (1.5.0)
       loofah (~> 2.19, >= 2.19.1)
-    railties (7.0.4.1)
-      actionpack (= 7.0.4.1)
-      activesupport (= 7.0.4.1)
+    railties (7.0.4.3)
+      actionpack (= 7.0.4.3)
+      activesupport (= 7.0.4.3)
       method_source
       rake (>= 12.2)
       thor (~> 1.0)
@@ -104,7 +104,8 @@ PLATFORMS
   x86_64-linux
 
 DEPENDENCIES
+  activesupport (= 7.0.4.3)
   govuk_app_config
 
 BUNDLED WITH

What is your fix for the problem, implemented in this PR?

Bundler is very conservative by default, trying to preserve versions from the lockfile as possible, and never downgrading them. However, when it runs into a resolution error, it still tries to find a valid resolution.

This fallback behavior was too "brute-force" though, completely unrestricting any gem found in the resolution conflict, and that could lead to direct dependencies being downgraded in some edge cases.

Instead, unlock things a bit more carefully:

  • First try unlocking fully pinned indirect dependencies, but leave a lower bound requirement in place to prevent downgrades.
  • Then try unlocking any fully pinned dependency, also leaving a lower bound requirement in place.
  • Finally completely unrestrict dependencies if nothing else worked.

Make sure the following tasks are checked

@deivid-rodriguez
Also set min locked requirements when not unlocking
3561ac2
@deivid-rodriguez
Fix unnecessary downgrade of top level dependency when unlocking
7f55ed8 
Bundler is very conservative by default, trying to preserve versions
from the lockfile as possible, and never downgrading them. However, when
it runs into a resolution error, it still tries to find a valid
resolution.

This fallback behavior was too "brute-force" though, completely
unrestricting any gem found in the resolution conflict, and that could
lead to direct dependencies being downgraded in some edge cases.

Instead, unlock things a bit more carefully:

* First try unlocking fully pinned indirect dependencies, but leave a
  lower bound requirement in place to prevent downgrades.
* Then try unlocking any fully pinned dependency, also leaving a lower
  bound requirement in place.
* Finally completely unrestrict dependencies if nothing else worked.
@deivid-rodriguez deivid-rodriguez mentioned this pull request Mar 23, 2023
Closed
1 task
@simi simi added this pull request to the merge queue Mar 24, 2023
Merged via the queue into master with commit b85e061 Mar 24, 2023
@simi simi deleted the no-downgrades branch March 24, 2023 20:22
deivid-rodriguez pushed a commit that referenced this pull request Mar 24, 2023
@simi @deivid-rodriguez
Merge pull request #6535 from rubygems/no-downgrades
ff07300 
Fix some unnecessary top level dependency downgrades

(cherry picked from commit b85e061)
@lauragift21 lauragift21 mentioned this pull request Apr 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simi simi simi approved these changes

Assignees
No one assigned
Labels
bundler: bug fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@deivid-rodriguez @simi