Add explicit source when available in the error msg upon failure #6211

papanikge · 2017-12-11T11:29:17Z

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

My bundling failed and the command that bundler output was wrong-ish, in the sense that my gem was suppose to come from a different source.

What was your diagnosis of the problem?

The error message was not explicit.

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

Bundler now explicitly print the source when there's just one. I think it's a good case just to print it when there's one. If there are more it's ambiguous, so we fallback to the old way. Maybe we could have a warning there that the command might not be exactly what the user wants.

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

See previous answer. I didn't find any easier ways of getting the source url.
Let me know if there are better implementations :)

ghost · 2017-12-11T11:29:19Z

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

colby-swandale · 2017-12-11T11:33:12Z

Thanks for opening a pull request. This will require a test before we consider merging.

colby-swandale · 2017-12-20T13:06:19Z

ping @papanikge

papanikge · 2018-01-03T09:52:29Z

sorry I didn't respond earlier. I will add a test.

papanikge · 2018-01-04T18:32:14Z

Hello, @colby-swandale et al,

I thought about it. I could stub generate_executable_stubs to raise an error and test that the message is what it should be in case of available remote source, but testing a string sounds way too much.
I don't know what else I could test. Any hints/ideas?

segiddins · 2018-01-05T02:36:50Z

See the specs in spec/install/failure_spec.rb, and copy like the first one, just adding an additional source to the gemfile?

papanikge · 2018-01-05T09:58:33Z

thanks for the tip @segiddins
I also fixed a broken spec I didn't see last time, and rebased over master...
take a look, if you will :)

papanikge · 2018-01-09T14:42:37Z

the broken test occurs because of the way those tests are written methinks. I could remove the extra spec and alter the message in the first to include the --source [...] part.

segiddins · 2018-01-13T04:54:05Z

the broken test occurs because of the way those tests are written methinks

how so?
it just looks like maybe the expected strings need to be updated

papanikge · 2018-01-13T15:12:46Z

@segiddins should I keep both then?

segiddins · 2018-01-15T19:47:54Z

If they're testing different things (i.e. have different setup or different assertions), then sure!

papanikge · 2018-01-24T09:37:13Z

It's now only broken for 2.5.0, which as far as I know is still under development.
Should I investigate?

colby-swandale · 2018-01-24T09:42:59Z

Yes, Ruby 2.5.0 was released to the general public nearly a month ago.

papanikge · 2018-01-24T10:04:35Z

ok, I had an old rbenv version. sorry about that 😁

colby-swandale · 2018-01-24T10:13:13Z

no worries 👍

The error-message function did not provide an explicit source, and that could lead in some confusion especially with big Gemfiles. The command that is output, should be valid.

papanikge · 2018-01-25T20:14:43Z

fixed the errors, rebased and squashed :)

papanikge · 2018-01-26T08:51:32Z

spec/commands/exec_spec.rb:759 passes locally on my system :(

colby-swandale · 2018-01-27T00:35:00Z

#6275 should fix this issue.

papanikge · 2018-01-28T17:48:24Z

so I guess I should wait until that is merged, and rebase over it?

segiddins · 2018-01-30T05:13:33Z

@papanikge no need to rebase, we can :+1 it via our mergebot once that PR lands!

segiddins · 2018-02-01T16:52:53Z

@bundlerbot r+

bundlerbot · 2018-02-01T16:52:54Z

📌 Commit 99d7fd1 has been approved by segiddins

bundlerbot · 2018-02-01T16:52:57Z

⌛ Testing commit 99d7fd1 with merge 8a32d64...

Add explicit source when available in the error msg upon failure ### What was the end-user problem that led to this PR? My bundling failed and the command that `bundler` output was wrong-ish, in the sense that my gem was suppose to come from a different source. ### What was your diagnosis of the problem? The error message was not explicit. ### What is your fix for the problem, implemented in this PR? Bundler now explicitly print the source when there's just one. I think it's a good case just to print it when there's one. If there are more it's ambiguous, so we fallback to the old way. Maybe we could have a warning there that the command might not be exactly what the user wants. ### Why did you choose this fix out of the possible options? See previous answer. I didn't find any easier ways of getting the source url. Let me know if there are better implementations :)

bundlerbot · 2018-02-01T17:33:06Z

☀️ Test successful - status-travis
Approved by: segiddins
Pushing 8a32d64 to master...

Add explicit source when available in the error msg upon failure ### What was the end-user problem that led to this PR? My bundling failed and the command that `bundler` output was wrong-ish, in the sense that my gem was suppose to come from a different source. ### What was your diagnosis of the problem? The error message was not explicit. ### What is your fix for the problem, implemented in this PR? Bundler now explicitly print the source when there's just one. I think it's a good case just to print it when there's one. If there are more it's ambiguous, so we fallback to the old way. Maybe we could have a warning there that the command might not be exactly what the user wants. ### Why did you choose this fix out of the possible options? See previous answer. I didn't find any easier ways of getting the source url. Let me know if there are better implementations :) (cherry picked from commit 8a32d64)

…rect Check if source responds to `#remotes` before printing gem install error message ### What was the end-user problem that led to this PR? There is a bug that was introduced in #6211 that prints an error message during `bundle install` if a gem failed to be installed. The error message attempts to call the `#remotes` method on `Bundler::Source::Git` which it does not implement. ### What was your diagnosis of the problem? See #6563 ### What is your fix for the problem, implemented in this PR? Check if the `source` responds to `#remotes` before and return early ### Why did you choose this fix out of the possible options? This seems to be the most simplest fix without having to refactor a lot of code.

…rect Check if source responds to `#remotes` before printing gem install error message ### What was the end-user problem that led to this PR? There is a bug that was introduced in #6211 that prints an error message during `bundle install` if a gem failed to be installed. The error message attempts to call the `#remotes` method on `Bundler::Source::Git` which it does not implement. ### What was your diagnosis of the problem? See #6563 ### What is your fix for the problem, implemented in this PR? Check if the `source` responds to `#remotes` before and return early ### Why did you choose this fix out of the possible options? This seems to be the most simplest fix without having to refactor a lot of code. (cherry picked from commit f358c36)

@eregon

## 1.16.5 (2018-09-18) Changes: - Add support for TruffleRuby (@eregon) Bugfixes: - Avoid printing git errors when checking the version on incorrectly packaged versions of Bundler ([#6453](rubygems/bundler#6453), @greysteil) - Fix issue where Bundler does not check the given class when comparing equality in DepProxy (@ChrisBr) - Handle `RangeNotSatisfiable` error in Compact Index (@MaxLap) - Check for initialized `search` variable in `LazySpecification` (@voxik) - Fix LoadError occurring in nested bundle exec calls ([#6537](rubygems/bundler#6537), @colby-swandale) - Check that Bundler::Deprecate is not an autoload constant ([#6163](rubygems/bundler#6163), @eregon) - Prefer non-pre-release versions when performing a `bundle update --patch` ([#6684](rubygems/bundler#6684), @segiddins) ## 1.16.4 (2017-08-17) Changes: - Welcome new members to the Bundler core team (@indirect) - Don't mutate original error trees when determining version_conflict_message (@greysteil) - Update vendored Molinillo to 0.6.6 (@segiddins) Bugfixes: - Reword bundle update regression message to be more clear to the user when a gem's version is downgraded ([#6584](rubygems/bundler#6584), @ralphbolo) - Respect --conservative flag when updating a dependency group ([#6560](rubygems/bundler#6560), @greysteil) - Fix issue where a pre-release version was not being selected when it's specified in the Gemfile ([#6449](rubygems/bundler#6449), @akihiro17) - Fix issue where `Etc` was not loaded when getting the user's home dir ([#6640](rubygems/bundler#6640), @colby-swandale) - Use UTF-8 for reading files including Gemfile ([#6660](rubygems/bundler#6660), @eregon) - Remove unnecessary `while` loop in path resolver helper (@ojab) Documentation: - Document that `bundle show [--paths]` sorts results by name (@kemitchell) ## 1.16.3 (2018-07-17) Features: - Support URI::File of Ruby 2.6 (@hsbt) Bugfixes: - Expand symlinks during setup to allow Bundler to load correctly when using symlinks in $GEM_HOME ([#6465](rubygems/bundler#6465), @ojab, @indirect) - Dont let Bundler create temporary folders for gem installs which are owned by root ([#6258](rubygems/bundler#6258), @colby-swandale) - Don't fallback to using temporary directories when needed directories already exist ([#6546](rubygems/bundler#6546), @brodock) - Use SharedHelpers.filesystem_access when reading a Gemfile so friendly error messages can be given to the user ([#6541](rubygems/bundler#6541), @segiddins) - Check if source responds to `#remotes` before printing gem install error message ([#6211](rubygems/bundler#6211), @colby-swandale) - Handle Errno::ENOTSUP in the Bundler Process Lock to prevent exceptions when using NFS mounts ([#6566](rubygems/bundler#6566), @colby-swandale) - Respect encodings when reading gemspecs ([#6598](rubygems/bundler#6598), @deivid-rodriguez) Documentation: - Fix links between manual pages (@BanzaiMan) - Add warning to Gemfile documentation for the use of the `source` option when declaring gems ([#6280](rubygems/bundler#6280), @forestgagnon) ## 1.16.2 (2018-04-20) Changes: - Include the gem's source in the gem install error message when available (@papanikge) - Remove unnecessary executable bit from gem template (@voxik) - Dont add the timestamp comment with gems added to the Gemfile via `bundle add` ([#6193](rubygems/bundler#6193), @cpgo) - Improve yanked gem error message (@alyssais) - Use `Bundler.rubygems.inflate` instead of the Gem::Util method directly (@segiddins) - Remove unused instance variable (@segiddins) Bugfixes: - Only trap INT signal and have Ruby's signal default handler be invoked (@shayonj) - Fix warning about the use of `__FILE__` in RubyGems integration testing (@MSP-Greg) - Skip the outdated bundler check when MD5 is not available ([#6032](rubygems/bundler#6032), @segiddins) - Fallback to the original error if the friendly message raises (@segiddins) - Rename Bundler.frozen? to avoid Object method conflict ([#6252](rubygems/bundler#6252), @segiddins) - Ensure the bindir exists before installing gems (@segiddins) - Handle gzip corruption errors in the compact index client ([#6261](rubygems/bundler#6261), @colby-swandale) - Check if the current directory is writeable when writing files in `bundle gem` ([#6219](rubygems/bundler#6219), @nilsding) - Fix hang when gemspec has incompatible encoding (@deivid-rodriguez) - Gracefully handle when the lockfile is missing spec entries for the current platform ([#6079](rubygems/bundler#6079), @segiddins) - Use Gem::Util.inflate instead of Gem.inflate (@hsbt) - Update binstub generator to use new ERB.new arity in Ruby 2.6 (@koic) - Fix `source_location` call in rubygems integration (@MSP-Greg) - Use `filesystem_access` when copying files in Compact Index Updater ([#6289](rubygems/bundler#6289), @segiddins) - Fail gracefully when resetting git gems to the given revision fails ([#6324](rubygems/bundler#6324), @segiddins) - Handle exceptions that do not have a backtrace ([#6342](rubygems/bundler#6342), @nesaulov) - Check if stderr was closed before writing to it (@shime) - Handle updating a specific gem for a non-local platform ([#6350](rubygems/bundler#6350), @greysteil) - Bump the `bundle_binstub` check-length to 300 characters (@tduffield) - Fix specifying alterntive Lockfile with `bundle lock` when default gemfile is present ([#6460](rubygems/bundler#6460), @agrim123) - Allow installing dependencies when the path is set to `.` ([#6475](rubygems/bundler#6475), @segiddins) - Support Bundler installing on a readonly filesystem without a home directory ([#6461](rubygems/bundler#6461), @grosser) - Filter git uri credentials in source description (@segiddins) Documentation: - Correct typos in `bundle binstubs` man page (@erikj, @samueloph) - Update links in `bundle gem` command documentation to use https (@KrauseFx) - Fix broken links between bundler man pages (@segiddins) - Add man page for the `bundle doctor` command ([#6243](rubygems/bundler#6243), @nholden) - Document `# frozen_string_literal` in `bundle init` Gemfile (@315tky) - Explain the gemspec files attribute in `bundle gem` template and print a link to bundler.io guides when running `bundle gem` ([#6246](rubygems/bundler#6246), @nesaulov) - Small copy tweaks & removed redundant phrasing in the bundler man page (@rubymorillo) - Improve the documentation of the settings load order in Bundler (@rubymorillo) - Added license info to main README (@rubymorillo) - Document parameters and return value of Injector#inject (@tobias-grasse)

papanikge force-pushed the add-source-when-failing branch from 41c97e9 to 6902f7e Compare January 5, 2018 09:57

Add explicit source when available in the error msg upon failure

99d7fd1

The error-message function did not provide an explicit source, and that could lead in some confusion especially with big Gemfiles. The command that is output, should be valid.

papanikge force-pushed the add-source-when-failing branch from 4255acc to 99d7fd1 Compare January 25, 2018 20:14

bundlerbot merged commit 99d7fd1 into rubygems:master Feb 1, 2018

colby-swandale added this to the 1.16.2 milestone Feb 4, 2018

colby-swandale mentioned this pull request Jun 8, 2018

Check if source responds to #remotes before printing gem install error message #6571

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explicit source when available in the error msg upon failure #6211

Add explicit source when available in the error msg upon failure #6211

papanikge commented Dec 11, 2017

ghost commented Dec 11, 2017

colby-swandale commented Dec 11, 2017

colby-swandale commented Dec 20, 2017

papanikge commented Jan 3, 2018

papanikge commented Jan 4, 2018 •

edited

Loading

segiddins commented Jan 5, 2018

papanikge commented Jan 5, 2018

papanikge commented Jan 9, 2018

segiddins commented Jan 13, 2018

papanikge commented Jan 13, 2018

segiddins commented Jan 15, 2018

papanikge commented Jan 24, 2018

colby-swandale commented Jan 24, 2018

papanikge commented Jan 24, 2018

colby-swandale commented Jan 24, 2018

papanikge commented Jan 25, 2018

papanikge commented Jan 26, 2018

colby-swandale commented Jan 27, 2018

papanikge commented Jan 28, 2018

segiddins commented Jan 30, 2018

segiddins commented Feb 1, 2018

bundlerbot commented Feb 1, 2018

bundlerbot commented Feb 1, 2018

bundlerbot commented Feb 1, 2018

Add explicit source when available in the error msg upon failure #6211

Add explicit source when available in the error msg upon failure #6211

Conversation

papanikge commented Dec 11, 2017

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

What was your diagnosis of the problem?

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

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

ghost commented Dec 11, 2017

colby-swandale commented Dec 11, 2017

colby-swandale commented Dec 20, 2017

papanikge commented Jan 3, 2018

papanikge commented Jan 4, 2018 • edited Loading

segiddins commented Jan 5, 2018

papanikge commented Jan 5, 2018

papanikge commented Jan 9, 2018

segiddins commented Jan 13, 2018

papanikge commented Jan 13, 2018

segiddins commented Jan 15, 2018

papanikge commented Jan 24, 2018

colby-swandale commented Jan 24, 2018

papanikge commented Jan 24, 2018

colby-swandale commented Jan 24, 2018

papanikge commented Jan 25, 2018

papanikge commented Jan 26, 2018

colby-swandale commented Jan 27, 2018

papanikge commented Jan 28, 2018

segiddins commented Jan 30, 2018

segiddins commented Feb 1, 2018

bundlerbot commented Feb 1, 2018

bundlerbot commented Feb 1, 2018

bundlerbot commented Feb 1, 2018

papanikge commented Jan 4, 2018 •

edited

Loading