Commits on Jul 29, 2016
  1. @homu

    Auto merge of #4812 - NickLaMuro:use_api_timeout_for_net_open_timeout…

    …, r=indirect
    
    Add an open_timeout to the fetcher connection
    
    Overview
    --------
    When ruby makes an HTTP connection with the `Net::HTTP` library, by default, it has no timeout to setup that connection.
    
    ```ruby
    # excerpts from net/http stdlib
    def initialize(address, port = nil)
      @address = address
      @port    = (port || HTTP.default_port)
      @local_host = nil
      @local_port = nil
      @curr_http_version = HTTPVersion
      @keep_alive_timeout = 2
      @last_communicated = nil
      @close_on_empty_response = false
      @socket  = nil
      @started = false
      @open_timeout = nil
      @read_timeout = 60
      # ...
    
    end
    
    def connect
      if proxy? then
        conn_address = proxy_address
        conn_port    = proxy_port
      else
        conn_address = address
        conn_port    = port
      end
    
      D "opening connection to #{conn_address}:#{conn_port}..."
      s = Timeout.timeout(@open_timeout, Net::OpenTimeout) {
        TCPSocket.open(conn_address, conn_port, @local_host, @local_port)
      }
      # ...
    
      if use_ssl?
        # ...
        Timeout.timeout(@open_timeout, Net::OpenTimeout) { s.connect }
      end
    end
    ```
    
    It can be set by changing the `open_timeout` variable on the connection object, but beyond that, it will attempt a connection unless an error is returned from the server.  The Net::HTTP::Persistent library included with bundler also maintains this same interface when dealing with threads, and eventually passes that same value down to the Net::HTTP lib.
    
    It is possible to get into a state where bundler is currently attempting to establish a connection indefinitely to rubygems because this timeout is not in place.  Using the `Fetcher.api_timeout`, which is just pulled from the `Bundler.settings[:timeout]` value (defaults to 10 sec), we can also provide an `open_timeout` value and prevent this.  In most cases, and HTTP connection should be established in less than a second (if not, there is probably a bigger problem), and most uses of the Fecther's connection are also wrapped in a Bundler::Retry, so if it fails do to a network hiccup, it will be attempted again.
    
    I was having this happen to me on a pretty consistent basis, and while it is probably more likely my ISP or something fishy with my home network, this seems like a relatively harmless fix.  In the code excerpts above from the STDLIB `net/http`, I would consistently have one or two Threads from the `CompactIndex` stall on the `s.connect`, which was while it was attempting to make an https connection.
    
    Unfortunately, I can't think of a good way to test or reproduce this consistently (and of course, now I can't even reproduce it locally at all), so this PR is more of posing a question of "Do we want to do this, and why or why not?"
    homu committed Jul 29, 2016
Commits on Jul 28, 2016
  1. @homu

    Auto merge of #4818 - NickLaMuro:better_net_http_reuse_for_compact_in…

    …dex, r=indirect
    
    Allow for Thread reuse for CompactIndex fetcher
    
    Overview
    --------
    One of the main benefits of the `Net::HTTP::Persistent` library is that it maintains the HTTP connections so that multiple requests to the same host don't need to re-instantiate the http tcp socket to make a request, and also allows this to be done across threads.  But when used with the `CompactIndex` client, the each iteration of the `remaining_gems?` loop throws away the `Bundle::Worker` and initiates new Threads with new connections.
    
    Since the resulting work on these threads only returns to the caller the result, and can be collected cleanly, it is safe to reuse these threads by other repeated `gem_name` iterations and gain a bit of a performance boost in the process.
    
    While in this code change, we do run `@bundle_worker.stop`, we could simple skip this now as we will never use more then one `Bundle::Worker`'s worth of threads, keeping around the worker for further calls to `.specs` on the same fetcher.  That said, this has not been don at this time.
    
    Metrics
    -------
    This is the result of running the fetcher for fetching gem metadata against the Gemfile for the [ManageIQ](https://github.com/ManageIQ/manageiq) project (these are sorted collection of results from the quickest times to the slowest times from a collection of benchmarks made over time, and are not in any other order besides that):
    
    | No changes | Shared threads, 25 workers  | Shared threads, 15 workers |
    | ---------- | --------------------------- | -------------------------- |
    |  14.780272 |                    4.429732 |                   4.424576 |
    |  14.987697 |                    4.664379 |                   4.447966 |
    |  16.161742 |                    7.470335 |                   4.596416 |
    |  16.698832 |                    8.538736 |                   8.103314 |
    |  16.973982 |                   10.293958 |                  10.368032 |
    |  17.044186 |                   12.548805 |                  10.873393 |
    |  17.401267 |                   13.527048 |                  11.734078 |
    |  17.417851 |                   13.640801 |                  12.379456 |
    |  17.611628 |                   13.695791 |                  14.071921 |
    |  17.702503 |                   13.930277 |                  14.151556 |
    |  17.926312 |                   14.512490 |                  14.247501 |
    |  17.953948 |                   14.547216 |                  14.372980 |
    |  18.116374 |                   14.562247 |                  14.494275 |
    |  18.193501 |                   15.054224 |                  14.496102 |
    |  18.278697 |                   16.325938 |                  14.496330 |
    |  18.313061 |                   16.773398 |                  14.799070 |
    |  18.561052 |                   17.029987 |                  15.377202 |
    |  18.657163 |                   17.321094 |                  15.658238 |
    |  18.661411 |                   17.443478 |                  17.018813 |
    |  18.739866 |                   17.895521 |                  17.142276 |
    |  19.008614 |                   18.075015 |                  17.414206 |
    |  19.174890 |                   18.159239 |                  17.860201 |
    |  19.235642 |                   18.188842 |                  18.460573 |
    
    The following changes to the `lib/bundler/source/rubygems.rb` were used to capture the metrics.
    
    ```diff
    diff --git a/lib/bundler/source/rubygems.rb b/lib/bundler/source/rubygems.rb
    index aedad70..2cd637b 100644
    --- a/lib/bundler/source/rubygems.rb
    +++ b/lib/bundler/source/rubygems.rb
    @@ -350,11 +350,17 @@ module Bundler
                 " Downloading full index instead..." unless allow_api
    
               if allow_api
    +            require "benchmark"
    +            puts Benchmark.measure {
                 api_fetchers.each do |f|
                   Bundler.ui.info "Fetching gem metadata from #{f.uri}", Bundler.ui.debug?
                   idx.use f.specs_with_retry(dependency_names, self)
                   Bundler.ui.info "" unless Bundler.ui.debug? # new line now that the dots are over
                 end
    +            }
    +
    +            puts; puts; puts "exiting..."
    +            exit
    ```
    
    And the command used to capture the metrics using this branch:
    
    ```
    $ for I in `seq 1 15`; do \
        BUNDLE_DISABLE_POSTIT=1 ruby -I /path/to/bundler/lib /path/to/bundler/exe/bundle update; \
      done
    ```
    
    Some things to note about this data:
    
    - These results are the lowest of the recorded and usable results, not an average
    - I went with the lowest because is show the range of improvement the best, but I didn't get an even number of usable results, so this was the best way (I found) to share a somewhat unbiased distribution
    - Usable results were the following
      - Used the `compact_index` resolver
      - Didn't require a retry
    - Unfortunately, almost half of my results had to be thrown away because they didn't match that criteria
    
    One thing that I also observed while running in debug node and inspecting the threads is the it seems that one or two workers tend take a majority of the requests, while the other workers seem to only process one.  This is why the metrics show a second column where I adjusted the number of workers spawned to 15 (no reflected in this code change), something to also consider.
    
    Tests
    -----
    Wasn't able to run tests locally when I using the latest version of master (not sure what broke), and unsure how I would test this functionality.  Let me know if this is something you would like to go forward with adding and I will look into adding some tests around it.
    homu committed Jul 29, 2016
  2. @homu

    Auto merge of #4822 - bundler:seg-feature-flag-trampoline, r=indirect

    Feature-flag the postit trampoline, disabled by default
    
    Set ENV["BUNDLE_ENABLE_TRAMPOLINE"] to enable it
    
    Closes #4821
    homu committed Jul 29, 2016
Commits on Jul 27, 2016
  1. @segiddins

    Feature-flag the postit trampoline, disabled by default

    Set ENV["BUNDLE_ENABLE_TRAMPOLINE"] to enable it
    segiddins committed Jul 27, 2016
  2. @NickLaMuro

    Allow for Thread reuse for CompactIndex fetcher

    One of the main benefits of the Net::HTTP persistent library is that it
    maintains the HTTP connections so that multiple requests to the same
    host don't need to re-instantiate the http tcp socket to make a request,
    and also allows this to be done across threads.
    
    But when used with the CompactIndex client, the each iteration of gem
    names throws away the `Bundle::Worker` and initiates new Threads with
    new connections.
    
    Since the resulting work on these threads only returns to the caller the
    result, and can be collected cleanly, it is safe to reuse these threads
    by other repeated `gem_name` iterations and gain a bit of a performance
    boost in the process.
    
    While in this code change, we do run `@bundle_worker.stop`, we could
    simple skip this now as we will never use more then one
    `Bundle::Worker`'s worth of threads, keeping around the worker for
    further calles to `.specs` on the same fetcher.
    NickLaMuro committed Jul 26, 2016
Commits on Jul 26, 2016
  1. @homu

    Auto merge of #4811 - NickLaMuro:dots_for_compact_index_logger, r=seg…

    …iddins
    
    Fix random string printing inconsistencies and formatting
    
    Add dot output for CompactIndex fetcher
    ---------------------------------------
    
    The `Bundler::Fetcher::CompactIndex`'s logging was missing the "dot logging" when not in debug mode, which is an assumed behavior based on the code in `lib/bundler/source/rubygems.rb`:
    
    ```ruby
    api_fetchers.each do |f|
      Bundler.ui.info "Fetching gem metadata from #{f.uri}", Bundler.ui.debug?
      idx.use f.specs_with_retry(dependency_names, self)
      Bundler.ui.info "" unless Bundler.ui.debug? # new line now that the dots are over
    end
    ```
    
    While this isn't critical for `bundler` function properly by any stretch of the imagination, it provides a small bit of user feedback that requests are still continuing to be processed and `bundler` hasn't stalled.  It also maintains logging consistency between the different fetcher models.
    
    Add newlines for `Bundler::Retry`
    ---------------------------------
    
    This is very pedantic, but it makes it so that retry warning messages that used to look like this:
    
    ```
    Fetching gem metadata from https://rubygems.org/....Retrying fetcher due to error (2/4): Error::OMGWatHappened LOL, no idea
    .....
    ```
    
    Now will look like this:
    
    ```
    Fetching gem metadata from https://rubygems.org/....
    Retrying fetcher due to error (2/4): Error::OMGWatHappened LOL, no idea.....
    ```
    
    I think this reads a bit better and puts context to the "dots" so they now match up with the original attempt and the retries
    
    Remove unneeded if statement
    ----------------------------
    
    Ran across this while failing at writing tests for the above (for longer than I want to admit in public), but basically the `if` statement to determine whether or not to print "name" in the `Bundler.ui.warn` was not needed, as it never could be reached because of a short circuiting return statement prior to the print
    
    ```ruby
    return true unless name
    ```
    
    Have no idea how to test it anymore than we already are, so instead of embarrassing myself further, I moved on.
    homu committed Jul 26, 2016
  2. @homu

    Auto merge of #4810 - NickLaMuro:fix_git_version_for_apple_git, r=seg…

    …iddins
    
    Removes OSX/msysgit strings from Bundler::Source::Git::GitProxy#version
    
    Overview
    --------
    
    When running the previous version of `Bundler::Source::Git::GitProxy#version` output through `Gem::Version.create` when the git version is something like
    
        $ git --version
        git version 1.2.3 (Apple Git-22)
    
    Ruby blows up with the error:
    
        ArgumentError:  Malformed version number string 1.2.3 (Apple Git-22)
    
    The '(Apple Git-22)' and any additions that are added by `msysgit` (ex: `1.8.3.msysgit.0`) can be safely ignored as they are additions to the git version string for those specific versions, but should still line up with the core git version.
    
    This regression was added in #4717, so this is not in a release of the gem yet, only in `1.13.0.rc1` installation.  This prevented:
    
    * running tests on OSX or on any OS with a non-core git installation
    * running `bundle install/update` with any git gem dependencies  in the Gemfile on OSX or any OS with a non-core git installation
    
    This change simply regexps those version additions out, in addition to removing the `git version`, so only the version number (containing numbers or `.`s) will be parsed by this regexp.
    
    Also, another method, `full_version` has been added to allow the `Bundler::Env` to report on the specific version of git being used, which is useful  when error reports are submitted.
    
    Note about tests
    ----------------
    The tests stub out any shell cmd and mocks a return value, which was easier to test the functionality of the regexp and requiring different git executable versions.  I also didn't add integration tests to address the regression caused by #4717, but simply tested that the versions parsed would work when sent through `Gem::Version.create`
    
    This does mean I only tested a sample of git version types, so there might be something that I missed.  Also, the test for `full_version` working in reports was kinda jank, but it was the best I could do...
    homu committed Jul 26, 2016
  3. @NickLaMuro

    Add an open_timeout to the fetcher connection

    When ruby makes an HTTP connection with the Net::HTTP library, but
    default, it has no timeout to setup that connection.  It can be set by
    changing the `open_timeout` variable on the connection object, but
    beyond that, it will attempt a connection unless an error is returned
    from the server.  The Net::HTTP::Persistent library included with
    bundler also maintains this same interface when dealing with threads,
    and eventually passes that same value down to the Net::HTTP lib.
    
    It is possible to get into a state where bundler is currently attempting
    to establish a connection indefinitely to rubygems because this timeout
    is not in place.  Using the `Fetcher.api_timeout`, which is just pulled
    from the `Bundler.settings[:timeout]` value (defaults to 10 sec), we can
    also provide an `open_timeout` value and prevent this.
    
    In most cases, and HTTP connection should be established in less than a
    second (if not, there is probably a bigger problem), and most uses of
    the Fecther's connection are also wrapped in a Bundler::Retry, so if it
    fails do to a network hiccup, it will be attempted again.
    NickLaMuro committed Jul 26, 2016
  4. @NickLaMuro

    Removes extra if in Bundler::Retry interpolation

    This particular if statement will never return false because it always
    preceded by `return true unless name`, so it will always be true if we
    reach that line.
    NickLaMuro committed Jul 25, 2016
  5. @NickLaMuro

    Update Bundler::Retry newlines

    When using Bundler::Retry in conjunction with "dots" for regular
    printing, the retry messages show up inline with the dots, then the dots
    continue on the next line.
    
    It reads a bit better to have the retry messages show up on a new line
    after the dots, then allow the dots to continue on the same line, so
    that the dots now represent the status for the retry.
    
    When in debug, just print the retry message as we always have.  The new
    line will assumed to not be necessary before the message, since the
    standard is to print newlines after everything in debug mode, and a
    newline will then also follow the retry message.
    NickLaMuro committed Jul 25, 2016
  6. @NickLaMuro

    Update CompactIndex to use log_specs

    `Bundler::Fetcher::CompactIndex` was missing the dots that are used by
    the fallback, `Bundler::Fetcher::Dependency` when running the `specs`
    method.  This simply makes use of the newly extracted `log_specs` method
    instead of simply calling out to `Bundler.ui.debug`.
    NickLaMuro committed Jul 25, 2016
  7. @NickLaMuro

    Move Bundler::Fetcher::Dependency#log_specs to Base

    Make log_specs a shared method between all fetchers, and require passing
    the debug message as the argument instead of the debug data, allowing it
    to be more universal.
    NickLaMuro committed Jul 25, 2016
  8. @homu

    Auto merge of #4808 - NickLaMuro:add_url_for_debug_http_response, r=s…

    …egiddins
    
    Add URI to http response output in debug mode
    
    Map request response to original URI when printing debug output for http responses for `bundler/fetcher/downloader`.
    
    Overview
    --------
    I have recently been debugging some bundler stalling issues and found this addition to the debug output helpful when ruling out if a particular URL was timing out with the `CompactIndex` fetcher as it does it's requests over 25 threads and returns them asynchronously.  Getting a bunch of `HTTP 304 Not Modified` isn't as helpful if you are trying to determine how a thread is locking up.
    
    Incidentally, this didn't end up helping solving some issues (further PRs will address this), but it seems like a simple debugging addition that could help rule out specific endpoint issues without requiring more [advanced debugging setups](https://gist.github.com/NickLaMuro/e923fc4e55307437a8f0e97ee6429410).
    homu committed Jul 26, 2016
  9. @NickLaMuro

    Use .full_version in Bundler::Env

    NickLaMuro committed Jul 25, 2016
  10. @NickLaMuro

    Add Bundler::Source::Git::GitProxy#full_version

    Adds a full_version method that can be used to display the OS specific
    info.  Useful when printing the environment information in the
    Bundler::Env
    NickLaMuro committed Jul 25, 2016
  11. @NickLaMuro

    Removes OSX/msysgit strings from git version

    When running the original `version` output through
    `Gem::Version.create` when the git version is something like
    
        $ git --version
        git version 1.2.3 (Apple Git-22)
    
    Ruby blows up with the error:
    
        ArgumentError:  Malformed version number string 1.2.3 (Apple Git-22)
    
    The '(Apple Git-22)' and any additions that are added by `msysgit` (ex:
    1.8.3.msysgit.0) can be safely ignored as they are additions to the git
    version for those specific versions.
    
    This was affecting running tests on OSX and being able to even bundle
    with git gem dependencies on OSX (with this version of the gem).
    
    This change simply regexes those rules out, in addition to removing the
    `git version`, so only the version number (containing numbers or `.`s)
    will be parsed.
    NickLaMuro committed Jul 25, 2016
  12. @homu

    Auto merge of #4800 - bundler:seg-locked-resolve-keep-platforms, r=in…

    …direct
    
    [Definition] Ensure gemspec dependencies include all lockfile platforms
    
    Closes #4798
    
    This is necessary since `gemspec`  declares platforms for the dependency as the reverse platform map of the generic local platform, thus missing out on all those platforms that aren't currently being resolved for
    homu committed Jul 26, 2016
Commits on Jul 25, 2016
  1. @homu

    Auto merge of #4802 - bundler:seg-fix-bundle-help, r=indirect

    Fix bundle --help
    
    Fixes the issue mentioned in #4801
    homu committed Jul 26, 2016
  2. @homu

    Auto merge of #4805 - bundler:seg-should-be-installed-source, r=indirect

    Add support for checking source in should_be_installed
    
    I was on a ✈️. I got bored. This got written. EOS
    homu committed Jul 26, 2016
  3. @NickLaMuro

    Find git version in spec helper with git_proxy

    This is minor, but duplicated code that also can be found in
    `lib/bundler/source/git/git_proxy.rb`, so lets use that so we are
    finding the git version in the same manner.  Bundler is fully loaded in
    the spec helper anyway, so this is already available to be used.
    NickLaMuro committed Jul 25, 2016
  4. @NickLaMuro

    Add URI to http response output in debug mode

    Useful when mapping completed responses to requests when debugging to
    determine which requests are still in progress.
    NickLaMuro committed Jul 25, 2016
  5. @segiddins

    Use reject! instead of select! for Ruby 1.8.7

    segiddins committed Jul 25, 2016
Commits on Jul 24, 2016
  1. @segiddins

    Update specs for improved gemspec detection

    segiddins committed Jul 24, 2016
  2. @segiddins

    Fix bundle --help

    segiddins committed Jul 22, 2016
  3. @segiddins

    Update a pair of specs to use the new source matching

    segiddins committed Jul 24, 2016
  4. @segiddins
  5. @segiddins
Commits on Jul 22, 2016
  1. @segiddins

    [DSL] Add support for multi-platform gems with the `gemspec` method

    segiddins committed Jul 22, 2016
  2. @segiddins

    Add install_gemfile! for specs

    segiddins committed Jul 22, 2016
  3. @segiddins
  4. @segiddins
  5. @segiddins
  6. @segiddins
  7. @segiddins
  8. @segiddins

    [Definition] Ensure gemspec dependencies include all lockfile platforms

    This avoids eliminating platform-specific gems from the lockfile when resolving on RUBY without any changes
    segiddins committed Jul 21, 2016