Allow Gem.default_user_install to be overridden

[voxik]

For Ruby re-distributors, the "auto-user-install" might be the right default. Therefore printing warning about installing into user directory is not always desirable. Let the "auto-user-install" feature to be customizable.

I have mentioned this is coming in #7236. But I have not really tested this yet.

And of course, the naming ... Not sure if the auto_user_install is the most descriptive method name.

Also, there is not test. Not sure if there should be one thought, because this will work only as long as it is not overridden by the Ruby executing the test suite. With such test case, we might end up with similar issues as #7187

[voxik]

After tinkering with this, I am less interested in the PR, while there still might be some merit. Let me explain.

There are historically two ways we used to configure RubyGems on Fedora for user installation:

1. Specifying --user-install via operating_system_defaults (which is similar to setting this option into .gemrc file): https://src.fedoraproject.org/rpms/ruby/blob/f195b6a3c84b26774c1a253bc8d002a4cdf37c1f/f/operating_system.rb#_103
2. Modifying Gem.dir, so it points to user home: https://src.fedoraproject.org/rpms/ruby/blob/87ddbf089e16006eec99fbf50d0b25ee8c1376fe/f/operating_system.rb

For the first case, the situation is similar as always specifying --user-install on command line, therefore there is no way users would be bothered by any distracting message. Or alternatively, the --no-user-install needs to be specified and in that case, user directory should not be used (once #7255) is merged.

For the second case, the Gem.dir == Gem.user_dir. Initially, I thought that if somebody deletes their ~/.local directory, that could be issue. But as it turns out, RubyGems creates the required directory structure prior it even gets close to the place, where the error would be reported. This removes my biggest concern. Nevertheless, the Defaulting to user installation might still be reported when the directory is not writeable, e.g. after I did something like $ sudo chown root:root -R ~/.local/share/.

So the question is if it is worth of it. The small benefit is that this PR would help to resolve one small corner case, where the chances this would bother anybody are really small.

OTOH, from the code readability POV, I think something like this would help and why not to make it configurable then?

At this point, I'm going to remove the WIP, because it probably is good as it is. If it is merged, then fine. If it is closed, then fine as well. Any other feedback is welcome as well 👼

[martinemde]

Seems like an improvement to the code regardless. It's much easier to read.

[martinemde]

Hmm, this was passing. I'm thinking there's a conflict between this and something we merged. Did I mess up the conflict resolution?

[deivid-rodriguez]

Yeah, not as pretty anymore but after the fix in #7255, you now want

diff --git a/lib/rubygems/installer.rb b/lib/rubygems/installer.rb
index 6eaf110e28..4c65468136 100644
--- a/lib/rubygems/installer.rb
+++ b/lib/rubygems/installer.rb
@@ -684,7 +684,7 @@ def process_options # :nodoc:
         # * `true`: `--user-install`
         # * `false`: `--no-user-install` and
         # * `nil`: option was not specified
-        if options[:user_install] || Gem.auto_user_install
+        if options[:user_install] || (options[:user_install].nil? && Gem.auto_user_install)
           @gem_home = Gem.user_dir
         end

or something equivalent.

[deivid-rodriguez]

Ha, you already fixed it! 😃

[martinemde]

Exactly. I updated it through a comment, though now I've made the commits fairly ugly in this PR.

[deivid-rodriguez]

Let me squash back to one commit, it's good to stress our CI a bit now that we merged #7267 :)

[voxik]

If you are happy even with the naming etc, them I am also happy 😊

[deivid-rodriguez]

Yeah, I'm also not sure it's the most descriptive name, maybe Gem.fallback_to_user_install?

[martinemde]

Ok, I updated the branch to fix my merge mistake and to change the method name. cc @voxik

[voxik]

With the method name, I was looking for consistency with the install_extension_in_lib, which is right bellow. But I am certainly not opposed to fallback_to_user_install? (even with question mark).

Otherwise it LGTM

[deivid-rodriguez]

I think we're coming up with a pretty neat version of this. How about this last tweak?

diff --git a/lib/rubygems/installer.rb b/lib/rubygems/installer.rb
index 4c65468136..124b3000d2 100644
--- a/lib/rubygems/installer.rb
+++ b/lib/rubygems/installer.rb
@@ -675,21 +675,17 @@ def process_options # :nodoc:
 
     @build_args = options[:build_args]
 
-    @gem_home = @install_dir
-
-    unless @gem_home
-      # `--build-root` overrides `--user-install` and auto-user-install
-      if @build_root.nil?
-        # Please note that `options[:user_install]` might have three states:
-        # * `true`: `--user-install`
-        # * `false`: `--no-user-install` and
-        # * `nil`: option was not specified
-        if options[:user_install] || (options[:user_install].nil? && Gem.auto_user_install)
-          @gem_home = Gem.user_dir
-        end
+    @gem_home = @install_dir || Gem.dir
+
+    # `--install-dir` and `--build-root` override `--user-install` and `Gem.fallback_to_user_install?`
+    if @install_dir.nil? && @build_root.nil?
+      # Please note that `options[:user_install]` might have three states:
+      # * `true`: `--user-install`
+      # * `false`: `--no-user-install` and
+      # * `nil`: option was not specified
+      if options[:user_install] || (options[:user_install].nil? && Gem.fallack_to_user_install?)
+        @gem_home = Gem.user_dir
       end
-
-      @gem_home ||= Gem.dir
     end
 
     # If the user has asked for the gem to be installed in a directory that is

[deivid-rodriguez]

With the method name, I was looking for consistency with the install_extension_in_lib, which is right bellow. But I am certainly not opposed to fallback_to_user_install? (even with question mark).

So you mean omitting the question mark for consistency? I'm good with that too.

[voxik]

How about default_user_install. Looking at the method and what can be achieved with it, if it is replaced by something else. E.g. this would be sensible override IMHO:

  def self.fallback_to_user_install? # :nodoc:
    false
  end

And that would mean that the old behavior is in place.

This could be possible implementation for Fedora, because we do this almost forever via different means.

  def self.fallback_to_user_install? # :nodoc:
      return true
  end

IOW the "fallback" is implied by the current implementation, but it might be changed by operating_system.rb or so.

[deivid-rodriguez]

Ok with me!

[deivid-rodriguez]

How about my suggestion at #7243 (comment) @voxik. Do you think it's an improvement?

[voxik]

I think we're coming up with a pretty neat version of this. How about this last tweak?

diff --git a/lib/rubygems/installer.rb b/lib/rubygems/installer.rb
index 4c65468136..124b3000d2 100644
--- a/lib/rubygems/installer.rb
+++ b/lib/rubygems/installer.rb
@@ -675,21 +675,17 @@ def process_options # :nodoc:
 
     @build_args = options[:build_args]
 
-    @gem_home = @install_dir
-
-    unless @gem_home
-      # `--build-root` overrides `--user-install` and auto-user-install
-      if @build_root.nil?
-        # Please note that `options[:user_install]` might have three states:
-        # * `true`: `--user-install`
-        # * `false`: `--no-user-install` and
-        # * `nil`: option was not specified
-        if options[:user_install] || (options[:user_install].nil? && Gem.auto_user_install)
-          @gem_home = Gem.user_dir
-        end
+    @gem_home = @install_dir || Gem.dir
+
+    # `--install-dir` and `--build-root` override `--user-install` and `Gem.fallback_to_user_install?`
+    if @install_dir.nil? && @build_root.nil?

This ^^ is the part I am most concerned with. IMHO, the current version can be read as it is written. E.g.

• It is clear that if --install-dir is specified, that is the winning option.
• But in case it was not specified, what we will do next?
• Ok, there might be --build-root option specified, which is another tricky option
• ...
• We handled all the exceptions and still nothing? Then go with the default.

But with what is proposed, I read the code such as:

• So there might be --install-dir specified and if it is not specified, then we go with default.
• But the condition if @install_dir.nil? && @build_root.nil?, I really cannot parse. We have dealt with --install-dir already, so why it is repeated here again? And what is relation with --build-root.

This disturbs the flow. It does not click for me. And I literally spend hours to wrap my head around.

[deivid-rodriguez]

I see --build-root and --install-dir as almost equivalent in my version, just that --install-dir also sets a new @gem_home. But any of them being set make --user-install and fallback behavior be skipped.

That's why it feels a bit weird to have a comment about --build-root, but not about --install-dir.

Anyways, I also get the way you look at it, let's discard my suggestion and move on with what we have.

[martinemde]

@voxik Want to do the update? I had fixed the branch to solve my conflict resolution but I don't want to keep force pushing your branches. 😉 If we switch the name to default_user_install I think we're all happy with it.

[voxik]

That's why it feels a bit weird to have a comment about --build-root, but not about --install-dir.

I was considering to remove the comment. To me, the comment was a sign of unreadable code. But since it was not mine, I decided to leave it there after all.

Anyways, I also get the way you look at it, let's discard my suggestion and move on with what we have.

👍 Thx

[voxik]

@voxik Want to do the update?

I'd be more then happy, but I won't do it sooner then tomorrow. If you like, please be my guest and force push my branch for one last time 😊

[martinemde]

@voxik Done.

[ioquatix]

Nice work team!

[deivid-rodriguez]

Funny how the code I suggested in #7243 (comment) is exactly what we had before #7236, I had not noticed 😅.

I came up with just one more attempt trying to please everyone, inspired on @skipkayhil's suggestion at #7236 (comment).