Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

fix error with path objects in array #6761

Merged
1 commit merged into from Oct 25, 2018
Merged

fix error with path objects in array #6761

1 commit merged into from Oct 25, 2018

Conversation

alexggordon
Copy link

Thanks so much for the contribution!
To make reviewing this PR a bit easier, please fill out answers to the following questions.

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

The problem was that Pathname instances in an array can not be sorted when there are string instances in the array also. Because of this, calling .sort before .to_s resulted in the error

[!] There was an error parsing `Gemfile`: comparison of Pathname with String failed. Bundler cannot continue.

you can easily see this issue doing

> require 'rails'
=> true
> [Pathname.new("/tmp/bundle")]
=> [#<Pathname:/tmp/bundle>]
> [Pathname.new("/tmp/bundle"), "test"]
=> [#<Pathname:/tmp/bundle>, "test"]
> [Pathname.new("/tmp/bundle"), "test"].sort
ArgumentError: comparison of Pathname with String failed

What was your diagnosis of the problem?

sort was called before map in the warn message.

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

We should call map(&:to_s) before calling sort, and add a test case for this scenario.

Why did you choose this fix out of the possible options?

Because it broke our production deploys.

@ghost
Copy link

ghost commented Oct 25, 2018

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

@alexggordon
Copy link
Author

alexggordon commented Oct 25, 2018

This fixes #6760 and #6758 I believe

Copy link
Contributor

@greysteil greysteil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - thanks for the speedy fix.

@greysteil
Copy link
Contributor

@bundlerbot r+

ghost pushed a commit that referenced this pull request Oct 25, 2018
6761: fix error with path objects in array r=greysteil a=alexggordon

Thanks so much for the contribution!
To make reviewing this PR a bit easier, please fill out answers to the following questions.

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

The problem was that `Pathname` instances in an array can not be sorted when there are string instances in the array also. Because of this, calling `.sort` before `.to_s` resulted in the error

```
[!] There was an error parsing `Gemfile`: comparison of Pathname with String failed. Bundler cannot continue.
```

you can easily see this issue doing
```
> require 'rails'
=> true
> [Pathname.new("/tmp/bundle")]
=> [#<Pathname:/tmp/bundle>]
> [Pathname.new("/tmp/bundle"), "test"]
=> [#<Pathname:/tmp/bundle>, "test"]
> [Pathname.new("/tmp/bundle"), "test"].sort
ArgumentError: comparison of Pathname with String failed
```

### What was your diagnosis of the problem?

`sort` was called before `map` in the warn message. 

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

We should call `map(&:to_s)` before calling sort, and add a test case for this scenario. 

### Why did you choose this fix out of the possible options?

Because it broke our production deploys. 

Co-authored-by: Alex Gordon <agordon@sessionm.com>
@alexggordon
Copy link
Author

You're welcome for the speedy fix! Any chance of this fix getting backported into 17.0 or getting a patch release pretty soon?

@ghost
Copy link

ghost commented Oct 25, 2018

Build succeeded

@ghost ghost merged commit 30c4426 into rubygems:master Oct 25, 2018
@greysteil
Copy link
Contributor

@colby-swandale is probably the best person to ask on that one, but we've had a few people run into this already so I wouldn't be opposed to a patch release in the next week.

@alexggordon
Copy link
Author

That would be awesome if possible!

@mateuscruz
Copy link

This broke our deploy pipeline earlier today. It took me 6 hours to identify the source and normalize all of our environments. I had to rollback to bundler 1.16. Thanks for the quick fix!

@greysteil
Copy link
Contributor

Sorry for the bug @mateuscruz! I can't promise a quick release, but I'll talk to the team and we'll do our best. We'll give it a few days to hoover up any other bugs either way.

indirect pushed a commit that referenced this pull request Oct 25, 2018
6761: fix error with path objects in array r=greysteil a=alexggordon

Thanks so much for the contribution!
To make reviewing this PR a bit easier, please fill out answers to the following questions.

The problem was that `Pathname` instances in an array can not be sorted when there are string instances in the array also. Because of this, calling `.sort` before `.to_s` resulted in the error

```
[!] There was an error parsing `Gemfile`: comparison of Pathname with String failed. Bundler cannot continue.
```

you can easily see this issue doing
```
> require 'rails'
=> true
> [Pathname.new("/tmp/bundle")]
=> [#<Pathname:/tmp/bundle>]
> [Pathname.new("/tmp/bundle"), "test"]
=> [#<Pathname:/tmp/bundle>, "test"]
> [Pathname.new("/tmp/bundle"), "test"].sort
ArgumentError: comparison of Pathname with String failed
```

`sort` was called before `map` in the warn message.

We should call `map(&:to_s)` before calling sort, and add a test case for this scenario.

Because it broke our production deploys.

Co-authored-by: Alex Gordon <agordon@sessionm.com>
@mateuscruz
Copy link

No worries, @greysteil . It was entirely our fault. We didn't have a locked version of bundler and it would update at every deploy. We've locked it to 1.16 for the time being. Keep up the good work.

@greysteil
Copy link
Contributor

@alexggordon @mateuscruz - this is now released in 1.17.1. Apologies for the regression (and all credit to @indirect for the incredibly quick release!).

colby-swandale added a commit that referenced this pull request Oct 29, 2018
* origin/1-17-stable:
  Version 1.17.1 with changelog
  Merge #6761
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Dec 17, 2018
pkgsr change
* Remove @Prefix@ from ALTERNATIVES file.

## 1.17.2 (2018-12-11)

 - Add compatability for bundler merge with Ruby 2.6

## 1.17.1 (2018-10-25)

 - Convert `Pathname`s to `String`s before sorting them, fixing #6760 and #6758 ([#6761](rubygems/bundler#6761), @alexggordon)

## 1.17.0 (2018-10-25)

No new changes.

## 1.17.0.pre.2 (2018-10-13)

Features:

  - Configure Bundler home, cache, config and plugin directories with `BUNDLE_USER_HOME`, `BUNDLE_USER_CACHE`, `BUNDLE_USER_CONFIG` and `BUNDLE_USER_PLUGIN` env vars ([#4333](rubygems/bundler#4333), @gwerbin)
  - Add `--all` option to `bundle binstubs` that will generate an executable file for all gems with commands in the bundle
  - Add `bundle remove` command to remove gems from the Gemfile via the CLI
  - Improve checking file permissions and asking for `sudo` in Bundler when it doesn't need to
  - Add error message to `bundle add` to check adding duplicate gems to the Gemfile
  - When asking for `sudo`, Bundler will show a list of folders/files that require elevated permissions to write to.

The following new features are available but are not enabled by default. These are intended to be tested by users for the upcoming release of Bundler 2.

  - Improve deprecation warning message for `bundle show` command
  - Improve deprecation warning message for the `--force` option in `bundle install`

## 1.17.0.pre.1 (2018-09-24)

Features:

  - Check folder/file permissions of the Bundle home directory in the `bundle doctor` command ([#5786](rubygems/bundler#5786), @ajwann)
  - Remove compiled gem extensions when running `bundle clean` ([#5596](rubygems/bundler#5596), @akhramov)
  - Add `--paths` option to `bundle list` command ([#6172](rubygems/bundler#6172), @colby-swandale)
  - Add base error class to gems generated from `bundle gem` ([#6260](rubygems/bundler#6260), @christhekeele)
  - Correctly re-install gem extensions with a git source when running `bundle pristine` ([#6294](rubygems/bundler#6294), @wagenet)
  - Add config option to disable platform warnings ([#6124](rubygems/bundler#6124), @agrim123)
  - Add `--skip-install` option to `bundle add` command to add gems to the Gemfile without installation ([#6511](rubygems/bundler#6511), @agrim123)
  - Add `--only-explicit` option to `bundle outdated` to list only outdated gems in the Gemfile ([#5366](rubygems/bundler#5366), @peret)
  - Support adding multiple gems to the Gemfile with `bundle add` ([#6543](rubygems/bundler#6543), @agrim123)
  - Make registered plugin events easier to manage in the Plugin API (@jules2689)
  - Add new gem install hooks to the Plugin API (@jules2689)
  - Add `--optimistic` and `--strict` options to `bundle add` ([#6553](https://github.com/bundler/bundler/issues/6553), @agrim123)
  - Add `--without-group` and `--only-group` options to `bundle list` ([#6564](rubygems/bundler#6564), @agrim123)
  - Add `--gemfile` option to the `bundle exec` command ([#5924](rubygems/bundler#5924), @ankitkataria)

The following new features are available but are not enabled by default. These are intended to be tested by users for the upcoming release of Bundler 2.

  - Make `install --path` relative to the current working directory ([#2048](rubygems/bundler#2048), @igorbozato)
  - Auto-configure job count ([#5808](rubygems/bundler#5808), @segiddins)
  - Use the Gem Version Promoter for major gem updates ([#5993](rubygems/bundler#5993), @segiddins)
  - Add config option to add the Ruby scope to `bundle config path` when configured globally (@segiddins)

## 1.16.6 (2018-10-05)

Changes:

  - Add an error message when adding a gem with `bundle add` that's already in the bundle ([#6341](rubygems/bundler#6341), @agrim123)
  - Add Homepage, Source Code and Chanagelog URI metadata fields to the `bundle gem` gemspec template (@walf443)

Bugfixes:

  - Fix issue where updating a gem resulted in the gem's version being downgraded when `BUNDLE_ONLY_UPDATE_TO_NEWER_VERSIONS` was set ([#6529](rubygems/bundler#6529), @theflow)
  - Fix some rescue calls that don't specifiy error type (@utilum)
  - Fix an issue when the Lockfile would contain platform-specific gems that it didn't need ([#6491](rubygems/bundler#6491), @segiddins)
  - Improve handlding of adding new gems with only a single group to the Gemfile in `bundle add` (@agrim123)
  - Refactor check for OpenSSL in `bundle env` (@voxik)
  - Remove an unnecessary assignment in Metadata (@voxik)

Documentation:

  - Update docs to reflect revised guidance to check in Gemfile.lock into version control for gems ([#5879](https://github.com/bundler/bundler/issues/5879), @arbonap)
  - Add documentation for the `--all` flag in `bundle update` (@agrim123)
  - Update README to use `bundle add` in usage examples (@hdf1986)
@colby-swandale colby-swandale added this to the 1.17.1 milestone Feb 25, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants