From a3260aa9d173388636a16e269da71bdb1464c9c9 Mon Sep 17 00:00:00 2001 From: Bundlerbot Date: Mon, 4 Nov 2019 11:04:25 +0000 Subject: Merge #7401 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7401: Stop silencing output by default r=indirect a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that bundler defaults to a silent UI: https://github.com/bundler/bundler/blob/e70643c1be3a4417bd537d7e63470265465e693e/lib/bundler.rb#L66-L68 In my opinion, this isn't a good behavior for a CLI tool, and forces us to override it in many many different places. It has also caused several issues, for example, https://github.com/bundler/bundler/issues/3710 where `bundle list` was printing nothing. The [solution to that issues](https://github.com/bundler/bundler/pull/3707) led us to add yet more places where we override the default UI, and @indirect [predicting that having to unset the UI everytime we want it to not be silent](https://github.com/bundler/bundler/pull/3707#issuecomment-108646127) would cause many headaches. Well, yeah... I've lost a lot of time trying to figure out why UI was silent sometimes, and normal another times, why some specs printed warnings and some didn't. In particular, see my series of "big fail PRs" fighting against bundler's UI: https://github.com/bundler/bundler/pull/7284, https://github.com/bundler/bundler/pull/7294, https://github.com/bundler/bundler/pull/7305, https://github.com/bundler/bundler/pull/7362. Another series of issues/PRs probably related to this is issue https://github.com/bundler/bundler/issues/6900, where the output would use a different UI on different environments. We had a lot of trouble to reliably fix it (https://github.com/bundler/bundler/pull/6994, https://github.com/bundler/bundler/pull/7002, https://github.com/bundler/bundler/issues/7253). I also run into these issues again when trying out the `RUBYGEMS_GEMDEPS` environment variable that enables `bundler/setup` from rubygems. ### What was your diagnosis of the problem? My diagnosis was that we shouldn't silence UI by default. ### What is your fix for the problem, implemented in this PR? My fix is to, instead of silencing and then overriding the default shell at many places, don't silence it by default and instead make it silent when needed. By doing this, I managed to get 100% of our specs green, so I'm pretty confident that the output is still the same (or if it's not, it's probably because some output/errors where being unintentionally swallowed). Now specs should pass, but they print a bunch of output to the screen. You can see error messages, hard crashes, success messages... Some of them might be showing actual issues with either the code or tests, so I plan to go through each of them and review them. I can do that in this PR or separately, no strong opinion. Co-authored-by: David Rodríguez (cherry picked from commit 1a585f5cd760ae4be6930fd38d262677faa3694c) --- lib/bundler.rb | 4 ++-- lib/bundler/cli.rb | 3 --- lib/bundler/cli/exec.rb | 7 ------- lib/bundler/friendly_errors.rb | 2 +- lib/bundler/gem_helper.rb | 1 - lib/bundler/inline.rb | 2 +- lib/bundler/rubygems_integration.rb | 1 - lib/bundler/setup.rb | 7 ++----- lib/bundler/shared_helpers.rb | 20 +------------------- 9 files changed, 7 insertions(+), 40 deletions(-) (limited to 'lib') diff --git a/lib/bundler.rb b/lib/bundler.rb index 4321a7ed3d..2ada6fe789 100644 --- a/lib/bundler.rb +++ b/lib/bundler.rb @@ -64,11 +64,11 @@ module Bundler end def ui - (defined?(@ui) && @ui) || (self.ui = UI::Silent.new) + (defined?(@ui) && @ui) || (self.ui = UI::Shell.new) end def ui=(ui) - Bundler.rubygems.ui = ui ? UI::RGProxy.new(ui) : nil + Bundler.rubygems.ui = UI::RGProxy.new(ui) @ui = ui end diff --git a/lib/bundler/cli.rb b/lib/bundler/cli.rb index bacbb8dbde..d7f749a672 100644 --- a/lib/bundler/cli.rb +++ b/lib/bundler/cli.rb @@ -22,9 +22,6 @@ module Bundler def self.start(*) super - rescue Exception => e # rubocop:disable Lint/RescueException - Bundler.ui = UI::Shell.new - raise e ensure Bundler::SharedHelpers.print_major_deprecations! end diff --git a/lib/bundler/cli/exec.rb b/lib/bundler/cli/exec.rb index 0b0e991ea5..0a1edbdbbd 100644 --- a/lib/bundler/cli/exec.rb +++ b/lib/bundler/cli/exec.rb @@ -43,15 +43,11 @@ module Bundler end def kernel_exec(*args) - ui = Bundler.ui - Bundler.ui = nil Kernel.exec(*args) rescue Errno::EACCES, Errno::ENOEXEC - Bundler.ui = ui Bundler.ui.error "bundler: not executable: #{cmd}" exit 126 rescue Errno::ENOENT - Bundler.ui = ui Bundler.ui.error "bundler: command not found: #{cmd}" Bundler.ui.warn "Install missing gem executables with `bundle install`" exit 127 @@ -62,15 +58,12 @@ module Bundler ARGV.replace(args) $0 = file Process.setproctitle(process_title(file, args)) if Process.respond_to?(:setproctitle) - ui = Bundler.ui - Bundler.ui = nil require_relative "../setup" TRAPPED_SIGNALS.each {|s| trap(s, "DEFAULT") } Kernel.load(file) rescue SystemExit, SignalException raise rescue Exception => e # rubocop:disable Lint/RescueException - Bundler.ui = ui Bundler.ui.error "bundler: failed to load command: #{cmd} (#{file})" backtrace = e.backtrace ? e.backtrace.take_while {|bt| !bt.start_with?(__FILE__) } : [] abort "#{e.class}: #{e.message}\n #{backtrace.join("\n ")}" diff --git a/lib/bundler/friendly_errors.rb b/lib/bundler/friendly_errors.rb index e9f06d90be..273573e820 100644 --- a/lib/bundler/friendly_errors.rb +++ b/lib/bundler/friendly_errors.rb @@ -16,7 +16,7 @@ module Bundler Bundler.ui.error error.message when GemRequireError Bundler.ui.error error.message - Bundler.ui.trace error.orig_exception, nil, true + Bundler.ui.trace error.orig_exception when BundlerError Bundler.ui.error error.message, :wrap => true Bundler.ui.trace error diff --git a/lib/bundler/gem_helper.rb b/lib/bundler/gem_helper.rb index d00c1894a8..81872b9429 100644 --- a/lib/bundler/gem_helper.rb +++ b/lib/bundler/gem_helper.rb @@ -26,7 +26,6 @@ module Bundler attr_reader :spec_path, :base, :gemspec def initialize(base = nil, name = nil) - Bundler.ui = UI::Shell.new @base = (base ||= SharedHelpers.pwd) gemspecs = name ? [File.join(base, "#{name}.gemspec")] : Dir[File.join(base, "{,*}.gemspec")] raise "Unable to determine name from existing gemspec. Use :name => 'gemname' in #install_tasks to manually set it." unless gemspecs.size == 1 diff --git a/lib/bundler/inline.rb b/lib/bundler/inline.rb index dbf737c7ee..5491555d67 100644 --- a/lib/bundler/inline.rb +++ b/lib/bundler/inline.rb @@ -56,7 +56,7 @@ def gemfile(install = false, options = {}, &gemfile) definition.missing_specs? end - Bundler.ui = ui if install + Bundler.ui = install ? ui : Bundler::UI::Silent.new if install || missing_specs.call Bundler.settings.temporary(:inline => true, :disable_platform_warnings => true) do installer = Bundler::Installer.install(Bundler.root, definition, :system => true) diff --git a/lib/bundler/rubygems_integration.rb b/lib/bundler/rubygems_integration.rb index 7549c592f1..c4950d14e8 100644 --- a/lib/bundler/rubygems_integration.rb +++ b/lib/bundler/rubygems_integration.rb @@ -635,7 +635,6 @@ module Bundler ENV["BUNDLE_GEMFILE"] ||= File.expand_path(gemfile) require_relative "gemdeps" runtime = Bundler.setup - Bundler.ui = nil activated_spec_names = runtime.requested_specs.map(&:to_spec).sort_by(&:name) [Gemdeps.new(runtime), activated_spec_names] end diff --git a/lib/bundler/setup.rb b/lib/bundler/setup.rb index d156f494a8..b9a2f8f1da 100644 --- a/lib/bundler/setup.rb +++ b/lib/bundler/setup.rb @@ -6,9 +6,8 @@ if Bundler::SharedHelpers.in_bundle? require_relative "../bundler" if STDOUT.tty? || ENV["BUNDLER_FORCE_TTY"] - Bundler.ui = Bundler::UI::Shell.new begin - Bundler.setup + Bundler.ui.silence { Bundler.setup } rescue Bundler::BundlerError => e Bundler.ui.warn "\e[31m#{e.message}\e[0m" Bundler.ui.warn e.backtrace.join("\n") if ENV["DEBUG"] @@ -18,12 +17,10 @@ if Bundler::SharedHelpers.in_bundle? exit e.status_code end else - Bundler.setup + Bundler.ui.silence { Bundler.setup } end # Add bundler to the load path after disabling system gems bundler_lib = File.expand_path("../..", __FILE__) $LOAD_PATH.unshift(bundler_lib) unless $LOAD_PATH.include?(bundler_lib) - - Bundler.ui = nil end diff --git a/lib/bundler/shared_helpers.rb b/lib/bundler/shared_helpers.rb index dec03ed160..6e83bc5ff4 100644 --- a/lib/bundler/shared_helpers.rb +++ b/lib/bundler/shared_helpers.rb @@ -137,10 +137,7 @@ module Bundler end return unless bundler_major_version >= major_version && prints_major_deprecations? - @major_deprecation_ui ||= Bundler::UI::Shell.new("no-color" => true) - with_major_deprecation_ui do |ui| - ui.warn("[DEPRECATED] #{message}") - end + Bundler.ui.warn("[DEPRECATED] #{message}") end def print_major_deprecations! @@ -217,21 +214,6 @@ module Bundler private - def with_major_deprecation_ui(&block) - ui = Bundler.ui - - if ui.is_a?(@major_deprecation_ui.class) - yield ui - else - begin - Bundler.ui = @major_deprecation_ui - yield Bundler.ui - ensure - Bundler.ui = ui - end - end - end - def validate_bundle_path path_separator = Bundler.rubygems.path_separator return unless Bundler.bundle_path.to_s.split(path_separator).size > 1 -- cgit v1.2.1