Now won't wait for dependencies that will never install. Partial fix for #3692 #4012

lukaso · 2015-09-24T20:43:36Z

Before this fix, if for any reason a dependency is not on the install list
it will cause a deadlock. This now means the bundle will no longer fail
in that instance, which matches the behavior of the sequential installer.
It isn't clear why the gem is not making it onto the list however, which
appears to be a separate bug.

This fixes my instance of #3692 which was still occurring despite the issue being closed.

Before this fix, if for any reason a dependency is not on the install list it will cause a deadlock. This now means the bundle will no longer fail in that instance, which matches the behavior of the sequential installer. It isn't clear why the gem is not making it onto the list however, which appears to be a separate bug.

lukaso · 2015-09-24T21:22:47Z

It also builds on this pr #3800. The original problem was that a dependency 'therubyracer' wasn't in the DEPENDENCIES section of the Gemfile.lock, which led to the deadlock condition. Now the parallel version will fail silently, just as the sequential version does.

Once I added therubyracer into the DEPENDENCIES section, my Gemfile.lock changed in the following way:

--- a/.bundle/config
+++ b/.bundle/config
@@ -1,6 +1,6 @@
 ---
 BUNDLE_PATH: vendor/bundle
 BUNDLE_DISABLE_SHARED_GEMS: '1'
-BUNDLE_JOBS: '4'
-BUNDLE_CACHE_ALL: true
-BUNDLE_CACHE_ALL_PLATFORMS: true
+BUNDLE_JOBS: 4
+BUNDLE_CACHE_ALL: 'true'
+BUNDLE_CACHE_ALL_PLATFORMS: 'true'
diff --git a/Gemfile.lock b/Gemfile.lock
index 8fef9e1..84c25f4 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -548,6 +548,7 @@ GEM
       addressable (~> 2.3)
     letter_opener (1.1.2)
       launchy (~> 2.2)
+    libv8 (3.16.14.11)
     listen (3.0.3)
       rb-fsevent (>= 0.9.3)
       rb-inotify (>= 0.9)
@@ -685,6 +686,7 @@ GEM
     redis (3.2.1)
     redis-namespace (1.5.2)
       redis (~> 3.0, >= 3.0.4)
+    ref (2.0.0)
     request_store (1.0.5)
     require_all (1.3.2)
     responders (1.1.2)
@@ -782,6 +784,9 @@ GEM
     teaspoon-jasmine (2.2.0)
       teaspoon (>= 1.0.0)
     terminal-table (1.5.2)
+    therubyracer (0.12.2)
+      libv8 (~> 3.16.14.0)
+      ref
     thor (0.19.1)
     thread_safe (0.3.5)
     tilt (1.4.1)

I'll see if there is an issue for this or open one.

segiddins · 2015-09-24T21:26:21Z

I'm just interested in how we even get into that invalid state to begin with... I'm tempted to add some informative assertions about the invarients around the installer to try and catch this

indirect · 2015-09-24T22:09:15Z

My best guess is git merges that create mutant lock files?

On Sep 24, 2015, 5:26 PM -0400, Samuel E. [email protected], wrote:

I'm just interested in how we even get into that invalid state to begin with... I'm tempted to add some informative assertions about the invarients around the installer to try and catch this

—
Reply to this email directly orview it on GitHub(#4012 (comment)).

segiddins · 2015-09-24T22:12:06Z

Ah, so we're trusting the lock file there to be correct, but in reality its missing some lines? Seems like a bug elsewhere in the installer then, and this is just a symptom

indirect · 2015-09-24T23:20:11Z

Yes and yes. I’m happy to accept this and at least back to the -j 1 brokenness rather than throw a deadlock, though, and investigate ways to detect bad lockfiles as a separate thing.

On Sep 25, 2015, at 12:12 AM, Samuel E. Giddins [email protected] wrote:

Ah, so we're trusting the lock file there to be correct, but in reality its missing some lines? Seems like a bug elsewhere in the installer then, and this is just a symptom

—
Reply to this email directly or view it on GitHub #4012 (comment).

segiddins · 2015-09-25T00:04:05Z

@indirect sounds good

lukaso · 2015-09-25T07:14:08Z

In my case it was definitely a bad merge. Detecting and throwing a better error is relatively straightforward in the parallel installer (throw an exception in the 'dependencies_installed?' method), but harder in the sequential. One option might be to get rid of the sequential installer and use parallel with one worker by default.

indirect · 2015-09-25T10:33:14Z

I like that idea, assuming that we’re able to make it so that the parallel installer with one worker actually works all the time. :)

On Sep 25, 2015, at 9:14 AM, Lukas Oberhuber [email protected] wrote:

In my case it was definitely a bad merge. Detecting and throwing a better
error is relatively straightforward in the parallel installer (throw an
exception in the 'dependencies_installed?' method), but harder in the
sequential. One option might be to get rid of the sequential installer and
use parallel with one worker by default.
—
Reply to this email directly or view it on GitHub #4012 (comment).

segiddins · 2015-09-26T02:03:18Z

CI failure: https://travis-ci.org/bundler/bundler/jobs/82041909#L251

segiddins · 2015-09-27T21:55:54Z

lib/bundler/installer/parallel_installer.rb

@@ -35,13 +35,14 @@ def ignorable_dependency?(dep)
    # sure needed dependencies have been installed.
    def dependencies_installed?(all_specs)
      installed_specs = all_specs.select(&:installed?).map(&:name)
-      dependencies.all? {|d| installed_specs.include? d.name }
+      dependencies(all_specs.map(&:name)).all? {|d| installed_specs.include? d.name }


pretty sure this change is breaking things?

@segiddins I don't see any failing tests for this line in Travis. Or am I missing something?

lukaso · 2015-09-28T08:44:11Z

Should I resubmit this PR against 1-10-stable? I realised that I didn't fully follow the bug fix procedure.

Also, there's no indication in the procedure (which I should have followed) about rubocop. Even on master where there's now a rake task.

segiddins · 2015-09-28T13:02:09Z

@lukaso well manually cherry pick back to stable once this is merged.

…egiddins Error on missing dependency - partial fix for #3692 (alternative) This pull request implements the strategy suggested in the comments for #4012, namely to raise an error when dependencies are not present (due to a corrupted `Gemfile.lock`). This also removes the `install_sequentially` option. I wasn't totally sure about the error message formatting.

homu · 2015-09-30T02:26:23Z

☔ The latest upstream changes (presumably #4020) made this pull request unmergeable. Please resolve the merge conflicts.

lukaso · 2015-09-30T08:01:54Z

This is superceded by #4020. Closing.

Fix rubocop offense

91f7d39

segiddins reviewed Sep 27, 2015
View reviewed changes

lukaso mentioned this pull request Sep 29, 2015

Error on missing dependency - partial fix for #3692 (alternative) #4020

Merged

lukaso closed this Sep 30, 2015

lukaso deleted the fix_parallel_installation_to_be_like_sequential branch September 30, 2015 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Now won't wait for dependencies that will never install. Partial fix for #3692 #4012

Now won't wait for dependencies that will never install. Partial fix for #3692 #4012

lukaso commented Sep 24, 2015

lukaso commented Sep 24, 2015

segiddins commented Sep 24, 2015

indirect commented Sep 24, 2015

segiddins commented Sep 24, 2015

indirect commented Sep 24, 2015

segiddins commented Sep 25, 2015

lukaso commented Sep 25, 2015 via email

indirect commented Sep 25, 2015

segiddins commented Sep 26, 2015

segiddins Sep 27, 2015

lukaso Sep 28, 2015

lukaso commented Sep 28, 2015

segiddins commented Sep 28, 2015

homu commented Sep 30, 2015

lukaso commented Sep 30, 2015

Now won't wait for dependencies that will never install. Partial fix for #3692 #4012

Now won't wait for dependencies that will never install. Partial fix for #3692 #4012

Conversation

lukaso commented Sep 24, 2015

lukaso commented Sep 24, 2015

segiddins commented Sep 24, 2015

indirect commented Sep 24, 2015

segiddins commented Sep 24, 2015

indirect commented Sep 24, 2015

segiddins commented Sep 25, 2015

lukaso commented Sep 25, 2015 via email

indirect commented Sep 25, 2015

segiddins commented Sep 26, 2015

segiddins Sep 27, 2015

Choose a reason for hiding this comment

lukaso Sep 28, 2015

Choose a reason for hiding this comment

lukaso commented Sep 28, 2015

segiddins commented Sep 28, 2015

homu commented Sep 30, 2015

lukaso commented Sep 30, 2015