[2.0] Stop remembering CLI options in Bundler 2 #5811

segiddins · 2017-06-22T23:18:55Z

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

The problem was that on Bundler 1.0, command line options passed to the different commands would be remembered across command invocations. This was hella confusing for users, and made Bundler a difficult command line tool to work with -- you'd pass an option once, it'd be set without you realizing it, and Bundler would behave differently.

See https://trello.com/c/yGsPNDpg/48-stop-auto-remembering-any-command-flags for the original discussion, but I've excerpted bits below:

Silently remembering flags that were passed to a command sometime in the past completely breaks all expectations for how command-line utilities work. It’s convenient for some users some of the time, but at the cost of many, many bugs filed that turn out to be unexpectedly remembered options.

Was was your diagnosis of the problem?

My diagnosis was that, behind a feature flag, we'd change commands behavior to only set settings temporarily (for the life of the current process), and after that disable options that become useless when not remembered.

See #3955.

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

My fix is to remove most CLI options in Bundler 2. Those that remain are set only for the duration of the process, rather than persisted to disk.

bundlerbot · 2017-06-23T03:07:52Z

☔ The latest upstream changes (presumably #5725) made this pull request unmergeable. Please resolve the merge conflicts.

segiddins · 2017-07-11T22:10:19Z

I think this is no longer a WIP, so feedback (particularly on which options are removed/kept in 2.0) would be 🚀

bundlerbot · 2017-07-12T21:55:11Z

☔ The latest upstream changes (presumably #5843) made this pull request unmergeable. Please resolve the merge conflicts.

segiddins · 2017-07-13T12:17:19Z

:all-of-the-conflicts:

colby-swandale

🎉

colby-swandale · 2017-07-18T02:27:08Z

lib/bundler/cli.rb

@@ -53,6 +52,11 @@ def initialize(*args)
      end
    end

+    def self.forgotten_option(*args, &blk)


Can i suggest removed_option or deprecate_option instead. This confused me a bit with the CLI no longer remembering options.

👍 I think I'll go with deprecated_option

colby-swandale · 2017-07-18T02:33:54Z

lib/bundler/installer/gem_installer.rb

@@ -66,6 +66,7 @@ def out_of_space_message
    end

    def generate_executable_stubs
+      return if Bundler.feature_flag.forget_cli_options?
      return if Bundler.settings[:inline]


return if Bundler.settings[:inline] || Bundler.feature_flag.forget_cli_options?

I prefer having the two separate conditions, since they're basically unrelated

segiddins · 2017-07-18T16:41:32Z

Rebased, addressed Colby's conflicts, fixed specs.
@indirect please review

indirect

I'd also like to deprecate init --gemspec, if that's okay with everyone else.

Generally looking great, thanks for taking the lead on shipping this stuff. 👍

indirect · 2017-07-19T01:30:05Z

lib/bundler/cli.rb

@@ -164,28 +168,28 @@ def check
      "Specify the number of jobs to run in parallel"
    method_option "local", :type => :boolean, :banner =>
      "Do not attempt to fetch gems remotely and use the gem cache instead"
-    method_option "no-cache", :type => :boolean, :banner =>
+    deprecated_option "no-cache", :type => :boolean, :banner =>
      "Don't update the existing gem cache."
    method_option "force", :type => :boolean, :banner =>
      "Force downloading every gem."
    method_option "no-prune", :type => :boolean, :banner =>


I think we should deprecate --no-prune... afaik it was only added because of --cache. Related, does the 2.0 cache command effectively run install and then fill vendor/cache? It should probably have a --prune/--no-prune flag, to offer equivalent functionality that cache-users can migrate to.

the 2.0 cache command is the 1.0 package command... it basically runs install && Bundler.load.cache and it already has a --no-prune flag. unrelated, but this reminds me we need to default cache_all_platforms to true in 2.0

indirect · 2017-07-19T01:32:12Z

lib/bundler/cli.rb

@@ -148,13 +152,13 @@ def check

      If the bundle has already been installed, bundler will tell you so and then exit.
    D
-    method_option "binstubs", :type => :string, :lazy_default => "bin", :banner =>
+    deprecated_option "binstubs", :type => :string, :lazy_default => "bin", :banner =>
      "Generate bin stubs for bundled gems to ./bin"
    method_option "clean", :type => :boolean, :banner =>
      "Run bundle clean automatically after install"


Can we deprecate this as well? I am pretty sure we can say "you can just run clean, which will install and then clean". The concept of cleaning isn't even really coherent without an install happening first, imo.

sure -- does this mean you'd like to default to cleaning or not after every install into a local path?

I would like to default to cleaning if and only if the bundle is being installed into the default application-local .bundle directory. If the user has provided some other "shared" location, I think cleaning should default to off. I'm worried that's possibly confusing, though. :/

indirect · 2017-07-19T01:35:01Z

lib/bundler/cli.rb

@@ -164,28 +168,28 @@ def check
      "Specify the number of jobs to run in parallel"
    method_option "local", :type => :boolean, :banner =>
      "Do not attempt to fetch gems remotely and use the gem cache instead"
-    method_option "no-cache", :type => :boolean, :banner =>
+    deprecated_option "no-cache", :type => :boolean, :banner =>
      "Don't update the existing gem cache."
    method_option "force", :type => :boolean, :banner =>
      "Force downloading every gem."


I feel like force is a very unclear name for this option, and I'd like to deprecate it in favor of something like --redownload. Or maybe better yet, a completely different command that nukes the cache, forcing downloads next time. (The latter is out of scope for this PR, though).

--redownload sounds good to me!

segiddins · 2017-07-19T14:26:09Z

I'd also like to deprecate init --gemspec, if that's okay with everyone else.

out of curiosity, why?

Since it did not cause the changed config to be written to disk

segiddins · 2017-07-19T19:04:19Z

@indirect think I got all of the ones you requested!

indirect · 2017-07-19T20:48:32Z

This looks good 👍 I'd like to deprecate init --gemspec because I think it should be replaced by using gemspec in the Gemfile.

segiddins · 2017-07-19T20:54:59Z

Cool! Just fixed the one test failure, so r?

indirect · 2017-07-19T20:56:48Z

@bundlerbot r+

bundlerbot · 2017-07-19T20:56:48Z

📌 Commit bc4b6c0 has been approved by indirect

bundlerbot · 2017-07-19T21:57:14Z

⌛ Testing commit bc4b6c0 with merge 53dfec6...

…irect [2.0] Stop remembering CLI options in Bundler 2 ### What was the end-user problem that led to this PR? The problem was that on Bundler 1.0, command line options passed to the different commands would be remembered _across command invocations_. This was hella confusing for users, and made Bundler a difficult command line tool to work with -- you'd pass an option once, it'd be set without you realizing it, and Bundler would behave differently. See https://trello.com/c/yGsPNDpg/48-stop-auto-remembering-any-command-flags for the original discussion, but I've excerpted bits below: > Silently remembering flags that were passed to a command sometime in the past completely breaks all expectations for how command-line utilities work. It’s convenient for some users some of the time, but at the cost of many, many bugs filed that turn out to be unexpectedly remembered options. ### Was was your diagnosis of the problem? My diagnosis was that, behind a feature flag, we'd change commands behavior to only set settings temporarily (for the life of the current process), and after that disable options that become useless when not remembered. See #3955. ### What is your fix for the problem, implemented in this PR? My fix is to remove most CLI options in Bundler 2. Those that remain are set only for the duration of the process, rather than persisted to disk.

bundlerbot · 2017-07-19T23:18:46Z

☀️ Test successful - status-travis
Approved by: indirect
Pushing 53dfec6 to master...

segiddins requested review from indirect and colby-swandale June 22, 2017 23:18

segiddins force-pushed the seg-stop-remembering-cli-options branch 4 times, most recently from b6532c5 to 9ded05f Compare July 11, 2017 20:35

segiddins changed the title ~~[WIP] [2.0] Stop remembering CLI options in Bundler 2~~ [2.0] Stop remembering CLI options in Bundler 2 Jul 11, 2017

segiddins force-pushed the seg-stop-remembering-cli-options branch from 9ded05f to 347015f Compare July 18, 2017 01:32

colby-swandale approved these changes Jul 18, 2017

View reviewed changes

segiddins force-pushed the seg-stop-remembering-cli-options branch from 347015f to 289306e Compare July 18, 2017 16:41

indirect reviewed Jul 19, 2017

View reviewed changes

segiddins added 13 commits July 19, 2017 13:10

[Settings] Stop remembering CLI options in Bundler 2

8ccf49c

[Settings] Remove #delete

e7d9e96

Since it did not cause the changed config to be written to disk

Update the settings specs for []= being removed

88f60e0

Remove unused Bundler.bundle_path= method

6b9defe

Set forgotten command line options via config in 2.0

aec2780

Remove several CLI flags that need to be set via config in Bundler 2

8a6400f

Remove several CLI flags that need to be set via config in Bundler 2

e21e08c

Use the create_file helper in the specs for its mkpath

50be118

Ensure groups are properly handled when the overlap in settings

88ed4a2

Fix deployment specs under Bundler 2

118e0d9

Fix the realworld specs under Bundler 2

79fb4b0

Update the specs for the new :deployment setting

9f55436

[CLI] Rename #forgotten_option to #deprecated_option

de3f2d2

segiddins added 2 commits July 19, 2017 13:12

[CLI] Deprecate init --gemspec

1b7955e

[CLI] Deprecate install --clean

9760e08

segiddins force-pushed the seg-stop-remembering-cli-options branch from 289306e to 4d47b28 Compare July 19, 2017 19:04

segiddins added 3 commits July 19, 2017 15:54

[CLI] Deprecate install --force in favor of --redownload

cb88c71

[CLI] Deprecate install --no-prune

4b81052

[EndpointSpecification] Ensure used ivars are initialized

bc4b6c0

segiddins force-pushed the seg-stop-remembering-cli-options branch from 4d47b28 to bc4b6c0 Compare July 19, 2017 20:54

bundlerbot merged commit bc4b6c0 into master Jul 19, 2017

This was referenced Jul 19, 2017

Add --only argument to bundle install #4907

Closed

Make install --path relative to the pwd #5803

Merged

segiddins deleted the seg-stop-remembering-cli-options branch July 20, 2017 15:41

denschub mentioned this pull request Oct 28, 2017

Incompatibility with Bundler 1.6 diaspora/diaspora#7653

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.0] Stop remembering CLI options in Bundler 2 #5811

[2.0] Stop remembering CLI options in Bundler 2 #5811

segiddins commented Jun 22, 2017 •

edited

bundlerbot commented Jun 23, 2017

segiddins commented Jul 11, 2017

bundlerbot commented Jul 12, 2017

segiddins commented Jul 13, 2017

colby-swandale left a comment

colby-swandale Jul 18, 2017

segiddins Jul 18, 2017

colby-swandale Jul 18, 2017

segiddins Jul 18, 2017

segiddins commented Jul 18, 2017

indirect left a comment

indirect Jul 19, 2017

segiddins Jul 19, 2017

indirect Jul 19, 2017

indirect Jul 19, 2017

segiddins Jul 19, 2017

indirect Jul 19, 2017

indirect Jul 19, 2017

segiddins Jul 19, 2017

indirect Jul 19, 2017

segiddins commented Jul 19, 2017

segiddins commented Jul 19, 2017

indirect commented Jul 19, 2017

segiddins commented Jul 19, 2017

indirect commented Jul 19, 2017

bundlerbot commented Jul 19, 2017

bundlerbot commented Jul 19, 2017

bundlerbot commented Jul 19, 2017

[2.0] Stop remembering CLI options in Bundler 2 #5811

[2.0] Stop remembering CLI options in Bundler 2 #5811

Conversation

segiddins commented Jun 22, 2017 • edited

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

Was was your diagnosis of the problem?

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

bundlerbot commented Jun 23, 2017

segiddins commented Jul 11, 2017

bundlerbot commented Jul 12, 2017

segiddins commented Jul 13, 2017

colby-swandale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

segiddins commented Jul 18, 2017

indirect left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

segiddins commented Jul 19, 2017

segiddins commented Jul 19, 2017

indirect commented Jul 19, 2017

segiddins commented Jul 19, 2017

indirect commented Jul 19, 2017

bundlerbot commented Jul 19, 2017

bundlerbot commented Jul 19, 2017

bundlerbot commented Jul 19, 2017

segiddins commented Jun 22, 2017 •

edited