This repository has been archived by the owner on Apr 14, 2021. It is now read-only.
Stop silencing output by default #7401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
indirect
approved these changes
Oct 31, 2019
This reverts commit 0a67ac4. And fixes deprecation warnings in another way not that the UI verbosity is predictable.
deivid-rodriguez
force-pushed
the
no_silent_output_by_default
branch
from
3181518
to
b8cd6ec
Compare
|
Thanks @indirect! I added a couple of extra tweaks so that the test output is clean again as it was PR this PR. It should be ready now I think. |
|
@bundlerbot r+ |
ghost
pushed a commit
that referenced
this pull request
Nov 1, 2019
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, #3710 where `bundle list` was printing nothing. The [solution to that issues](#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](#3707 (comment)) 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: #7284, #7294, #7305, #7362. Another series of issues/PRs probably related to this is issue #6900, where the output would use a different UI on different environments. We had a lot of trouble to reliably fix it (#6994, #7002, #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 <deivid.rodriguez@riseup.net>
|
@bundlerbot retry |
ghost
pushed a commit
that referenced
this pull request
Nov 4, 2019
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, #3710 where `bundle list` was printing nothing. The [solution to that issues](#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](#3707 (comment)) 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: #7284, #7294, #7305, #7362. Another series of issues/PRs probably related to this is issue #6900, where the output would use a different UI on different environments. We had a lot of trouble to reliably fix it (#6994, #7002, #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 <deivid.rodriguez@riseup.net>
Build succeeded |
ghost
merged commit
ghost
pushed a commit
that referenced
this pull request
Nov 6, 2019
7426: Revert "I don't think we need this now" r=deivid-rodriguez a=deivid-rodriguez This reverts commit 3681686. ### What was the end-user problem that led to this PR? The problem was that in #7401, after I got specs passing against a non-silenced by default UI, I started undoing workarounds that I thought were due to the silent by default UI. ### What was your diagnosis of the problem? My diagnosis was that in the case I'm reverting here this is not actually fixed, [as the Azure Pipelines logs confirm](https://dev.azure.com/bundler/bundler/_build/results?buildId=2662&view=ms.vss-test-web.build-test-results-tab&runId=1003360&resultId=101526&paneView=debug). ### What is your fix for the problem, implemented in this PR? My fix is to unrevert the specific commit that I shouldn't have reverted. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
deivid-rodriguez
pushed a commit
that referenced
this pull request
Nov 7, 2019
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, #3710 where `bundle list` was printing nothing. The [solution to that issues](#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](#3707 (comment)) 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: #7284, #7294, #7305, #7362. Another series of issues/PRs probably related to this is issue #6900, where the output would use a different UI on different environments. We had a lot of trouble to reliably fix it (#6994, #7002, #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 <deivid.rodriguez@riseup.net> (cherry picked from commit 1a585f5)
deivid-rodriguez
pushed a commit
that referenced
this pull request
Nov 7, 2019
7426: Revert "I don't think we need this now" r=deivid-rodriguez a=deivid-rodriguez This reverts commit 3681686. ### What was the end-user problem that led to this PR? The problem was that in #7401, after I got specs passing against a non-silenced by default UI, I started undoing workarounds that I thought were due to the silent by default UI. ### What was your diagnosis of the problem? My diagnosis was that in the case I'm reverting here this is not actually fixed, [as the Azure Pipelines logs confirm](https://dev.azure.com/bundler/bundler/_build/results?buildId=2662&view=ms.vss-test-web.build-test-results-tab&runId=1003360&resultId=101526&paneView=debug). ### What is your fix for the problem, implemented in this PR? My fix is to unrevert the specific commit that I shouldn't have reverted. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net> (cherry picked from commit b12a21f)
Merged
ghost
pushed a commit
that referenced
this pull request
Nov 8, 2019
7428: 2.1.0.pre.3 r=deivid-rodriguez a=deivid-rodriguez This is the upcoming 2.1.0.pre.3. I only documented PRs that (I expect to) include user facing changes, but the release also includes: * Some improvements in bundler integration as a default gem. I don't think these changes should change behavior but there's some default gem integration issues that I could never reproduce but the less we rely and mess with bundler's own `$LOAD_PATH` usually help here. That's #7398. * Improvements in the experience debugging bundler by making the UI non silent by default. Again, I don't expect user facing changes, but if I'm missing something, I think it would be for the better. This is #7401. * Bumping fileutils version to 1.3.0. We were using 1.2.0 with some patches on top. Again, no expected changes on the user side, but we should aim that our releases include vendorize versions of dependencies that exactly match something released on rubygems.org, and this change goes in that direction. This is #7375. * Refactorings and updates to the development environment. Co-authored-by: Bundlerbot <bot@bundler.io>
ghost
pushed a commit
that referenced
this pull request
Nov 21, 2019
7442: Fix `bundle exec`'ing to rubygems being silent r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that #7401 caused a regression where `bundle exec`'ing to rubygems would silence rubygems output. ### What was your diagnosis of the problem? My diagnosis was that the removal of: * Code where `Bundler::RGProxy` would be only set conditionally on `Bundler.ui =` not being passed `nil`. * Code setting `Bundler.ui` to `nil` right before shelling during `bundle exec`. caused rubygems UI to be silent during `bundle exec gem`. ### What is your fix for the problem, implemented in this PR? My fix is to explictly "unsilence" rubygems UI before `bundle exec` calls. ### Why did you choose this fix out of the possible options? I chose this fix because it's more explicit than the previous one. Fixes #7441. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
deivid-rodriguez
pushed a commit
that referenced
this pull request
Dec 13, 2019
7442: Fix `bundle exec`'ing to rubygems being silent r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that #7401 caused a regression where `bundle exec`'ing to rubygems would silence rubygems output. ### What was your diagnosis of the problem? My diagnosis was that the removal of: * Code where `Bundler::RGProxy` would be only set conditionally on `Bundler.ui =` not being passed `nil`. * Code setting `Bundler.ui` to `nil` right before shelling during `bundle exec`. caused rubygems UI to be silent during `bundle exec gem`. ### What is your fix for the problem, implemented in this PR? My fix is to explictly "unsilence" rubygems UI before `bundle exec` calls. ### Why did you choose this fix out of the possible options? I chose this fix because it's more explicit than the previous one. Fixes #7441. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net> (cherry picked from commit a11c104)
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, #3710 where
bundle listwas printing nothing.The solution to that issues 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 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: #7284, #7294, #7305, #7362.
Another series of issues/PRs probably related to this is issue #6900, where the output would use a different UI on different environments. We had a lot of trouble to reliably fix it (#6994, #7002, #7253).
I also run into these issues again when trying out the
RUBYGEMS_GEMDEPSenvironment variable that enablesbundler/setupfrom 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.