Allow user to specify a PATH to search for gem overrides #34

kbrock · 2023-12-19T18:22:11Z

This uses a PATH to locate an override gem.

The idea behind this is a developer has a common directory where they git clone gems and tweak them.

I think the 2 outstanding issues are:

What do we call the override path?
~~When do we not want to override a path? (maybe the developer has a gem checked out but they are overriding it with a github repo?)~~ SOLUTION: only override if no options specified (or they provide a partial path)

kbrock · 2023-12-19T18:28:56Z

update:

s/(File.)exists/exist/g

(annoyingly, I had added File.exists? and File.exist?)

agrare · 2023-12-19T18:42:18Z

lib/bundler/inject/dsl_patch.rb

@@ -97,6 +99,10 @@ def load_bundler_d(dir)
          eval_gemfile(f)
        end
      end
+
+      def search_path(calling_file)
+        [File.dirname(calling_file)] + ENV.fetch("BUNDLE_BUNDLER_INJECT__GEM_PATH", "").split(':')


We probably should specify in the README what the search order is, I think this is perfect though prioritize the local dir then in order in the BUNDLE_BUNDLER_INJECT__GEM_PATH

Minor, but let's move any env var handling to the same place that skip_warnings? is, and wrap it in a proper method.

Main reason is we want as little methods here as possible because this patches into Bundler::Dsl, so pure functional methods like this can live elsewhere.

Also, once you move over there, you'll see the preferable way to access the settings is with Bundler.settings["bundler_inject.gem_path"]. This allows all of ENV var, .bundle/config file, global bundler config, and local bundler config.

Fryguy · 2023-12-19T18:54:26Z

Please also include some README docs next to the current docs for skip warnings.

Separately, I'm not sure about gem_path as the setting name, but I can't think of anything better offhand...I'll think about it.

Fryguy · 2023-12-19T18:55:53Z

I tend to like explicitness with these kinds of options. My first thought was override_gem_default_path (though if this is also used by ensure_gem, then that doesn't work)

Fryguy · 2023-12-19T19:03:00Z

spec/bundler_inject_spec.rb

+            write_bundler_d_file <<~F
+              override_gem "ansi", :path => "the_gem"
+            F
+            bundle(:update, env: { "BUNDLE_BUNDLER_INJECT__GEM_PATH" => "#{path.dirname.to_s}/"})


I think the .to_s here is redundant.

Also why the trailing slash? I wouldn't expect a caller to do that (unless we have a separate test for trailing slash and not trailing slash to support both.

I removed the interpolation, but ended up needing the to_s again. (blew up without it)

Right, if interpolating you don't need the to_s, but if you removed the interpolation, then you do need it.

Fryguy · 2023-12-19T19:06:18Z

lib/bundler/inject/dsl_patch.rb

-        args.last[:path] = File.expand_path(args.last[:path], File.dirname(calling_file))
+      def expand_gem_path(name, args, calling_file)
+        args << {} unless args.last.kind_of?(Hash)
+        path_specified = args.last[:path] || name


There are cases where we do override gem but only want to override the version (or some other non-path value), and not the path. How would we avoid this automatic path detection in that case?

I guess not setting the setting could be the answer, and if you have the setting, then we assume all overrides are path based? If so, we need to detail that nuance in the readme.

I avoided looking to into the args.
We now skip if version or anything is specified

kbrock · 2023-12-19T22:34:38Z

update:

only add a path if there is no version or any other argument
use bundler_inject.gem_path or $BUNDLE_BUNDLER_INJECT__GEM_PATH
use File::PATH_SEPARATOR
using hashrocket for env
no longer fix coverage_helper fix (another PR)

update:

added README documentation
rubocops

update:

add Bundler::Inject.gem_path
Path.to_s in specs (fix errors)

update:

fix trailing whitespace

Fryguy · 2023-12-20T21:57:14Z

Can you update the PR title to reflect what you are doing here?

Fryguy · 2023-12-20T21:59:27Z

lib/bundler/inject.rb

+      #   BUNDLE_BUNDLER_INJECT__GEM_PATH=~/src:~/gems_src
+      #
+      def gem_path
+        @gem_path ||= (Bundler.settings["bundler_inject.gem_path"] || "").split(File::PATH_SEPARATOR)


Should we also .map { |p| File.expand_path(p) }, on the end of this to expand any ~?

Fryguy · 2023-12-20T22:02:30Z

spec/support/helpers.rb

@@ -92,14 +92,14 @@ def rm_global_bundler_d_dir
      FileUtils.rm_rf(Helpers.global_bundler_d_dir)
    end

-    def with_path_based_gem(source_repo)
+    def with_path_based_gem(source_repo, dir_name = "the_gem")


I noticed a bunch of the callers were changed (e.g. spec/bundler_inject_spec.rb:121) to set the dir_name at the caller side. If they are all hardcoded at the caller, do we still need the default value here anymore (hard to tell if they were all changed)?

oops. I stated this below.
I think this is only called from a few spots. I will remove the default.

Fryguy · 2023-12-20T22:02:52Z

spec/bundler_inject_spec.rb

-        it "with a path" do
-          with_path_based_gem("https://github.com/rubyworks/ansi") do |path|
+        it "with an absolute path" do
+          with_path_based_gem("https://github.com/rubyworks/ansi", "the_gem") do |path|


More out of curiosity, why was this hardcoded to the parameter default?

I had it as the default in the helper method because I wanted the interface to be backwards compatible.
In this spec, it referenced "the_gem", so I felt it was good to have this string in the spec.

We could probably drop the default.

Fryguy · 2023-12-20T22:04:02Z

spec/bundler_inject_spec.rb

+            write_bundler_d_file <<~F
+              override_gem "ansi", :path => "the_gem"
+            F
+            bundle(:update, :env => {"BUNDLE_BUNDLER_INJECT__GEM_PATH" => path.dirname.to_s})


Can you add a spec that shows multiple paths in the ENV var, and then something appearing in not the first path? This would exercise the File::PATH_SEPARATOR code as well as that the map picks a later value.

Fryguy · 2023-12-20T22:06:39Z

lib/bundler/inject/dsl_patch.rb

@@ -97,6 +100,10 @@ def load_bundler_d(dir)
          eval_gemfile(f)
        end
      end
+
+      def search_path(gemfile_dir)


I'm wondering if this should just be inlined into the expand_gem_path method, since it's not being reused anywhere else. The reason is that this method will be patched into the Bundler::Dsl module, and the generic name of the method concerns me that it might conflict in the future.

I'm also concerned about gem_path. I will change that as well.

Before ------ Given a relative path to an overriding gem, use the Gemfile directory to locate a gem After ----- Given a relative path (or no path at all - assume it is the gem name), use the Gemfile directory and BUNDLE_BUNDLER_INJECT__GEM_PATH directories to locate a gem

kbrock · 2024-01-03T16:04:52Z

update:

rebased master (to fix conflict)
inlined gem_path into search_path
renamed search_path to bundler_inject_search_path (to avoid possible collision)

update:

added spec with multiple directories

update:

remove default name for helper method with_path_based_gem
inline search_path into expand_gempath
extract bundler_inject_gem_path -- misunderstood inline comment above

orangewolf · 2024-01-08T19:45:31Z

found this plugin, which is great for a fairly odd usecase we have, but my first thought was that we'd want a custom path. this seems like a well baked solution to me! thank you for it =-)

agrare

LGTM

agrare · 2024-01-09T17:22:32Z

@orangewolf glad you found it useful!

but my first thought was that we'd want a custom path

You could pass paths as an option before so for example: override_gem "manageiq-api", :path => "~/src/manageiq/manageiq-api" and this path could be anywhere. This PR helps if you have a lot of overrides and they all tend to live in the same dir, you don't have to keep repeating yourself :)

Fixed * Remove remnants of bundler 1.x support (#20) * Fix detection of bundler version (#21) * Fix spec issues after release of bundler 2.4.17 (#32) Added * Switch to GitHub Actions (#25) * Add bundler 2.4 to the test matrix (#28) * Add Ruby 3.1 and 3.2 to test matrix (#29) * Allow user to specify a PATH to search for gem overrides (#34)

kbrock added the enhancement New feature or request label Dec 19, 2023

kbrock assigned Fryguy Dec 19, 2023

kbrock force-pushed the gem_path branch from db76055 to 43cf4be Compare December 19, 2023 18:26

agrare reviewed Dec 19, 2023

View reviewed changes

Fryguy reviewed Dec 19, 2023

View reviewed changes

kbrock force-pushed the gem_path branch 2 times, most recently from d3de88d to 82ee2fc Compare December 19, 2023 22:16

kbrock force-pushed the gem_path branch 2 times, most recently from edda056 to d2cf9e6 Compare December 19, 2023 22:53

Fryguy reviewed Dec 20, 2023

View reviewed changes

kbrock changed the title ~~Gem path~~ Allow user to specify a PATH to search for gem overrides Jan 3, 2024

kbrock force-pushed the gem_path branch 2 times, most recently from 4151483 to 7fe6e23 Compare January 3, 2024 15:19

kbrock added 2 commits January 3, 2024 10:47

Allow with_path_based_gem to specify the directory name

1028ccf

kbrock force-pushed the gem_path branch from 7fe6e23 to 6a9ebc9 Compare January 3, 2024 15:49

agrare approved these changes Jan 9, 2024

View reviewed changes

agrare merged commit c1b0037 into ManageIQ:master Jan 9, 2024
19 checks passed

agrare assigned agrare and unassigned Fryguy Jan 9, 2024

kbrock deleted the gem_path branch January 16, 2024 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user to specify a PATH to search for gem overrides #34

Allow user to specify a PATH to search for gem overrides #34

kbrock commented Dec 19, 2023 •

edited

Loading

kbrock commented Dec 19, 2023

agrare Dec 19, 2023

Fryguy Dec 19, 2023 •

edited

Loading

Fryguy Dec 19, 2023

Fryguy commented Dec 19, 2023

Fryguy commented Dec 19, 2023

Fryguy Dec 19, 2023 •

edited

Loading

kbrock Dec 19, 2023

Fryguy Dec 20, 2023

Fryguy Dec 19, 2023 •

edited

Loading

Fryguy Dec 19, 2023

kbrock Dec 19, 2023

kbrock commented Dec 19, 2023 •

edited

Loading

Fryguy commented Dec 20, 2023

Fryguy Dec 20, 2023

Fryguy Dec 20, 2023

kbrock Jan 3, 2024

Fryguy Dec 20, 2023

kbrock Jan 3, 2024

Fryguy Dec 20, 2023

Fryguy Dec 20, 2023

kbrock Jan 3, 2024

kbrock commented Jan 3, 2024

orangewolf commented Jan 8, 2024

agrare left a comment

agrare commented Jan 9, 2024

Allow user to specify a PATH to search for gem overrides #34

Allow user to specify a PATH to search for gem overrides #34

Conversation

kbrock commented Dec 19, 2023 • edited Loading

kbrock commented Dec 19, 2023

Choose a reason for hiding this comment

Fryguy Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Dec 19, 2023

Fryguy commented Dec 19, 2023

Fryguy Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Dec 19, 2023 • edited Loading

Fryguy commented Dec 20, 2023

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

kbrock commented Jan 3, 2024

orangewolf commented Jan 8, 2024

agrare left a comment

Choose a reason for hiding this comment

agrare commented Jan 9, 2024

kbrock commented Dec 19, 2023 •

edited

Loading

Fryguy Dec 19, 2023 •

edited

Loading

Fryguy Dec 19, 2023 •

edited

Loading

Fryguy Dec 19, 2023 •

edited

Loading

kbrock commented Dec 19, 2023 •

edited

Loading