Run Bundler v1 native helpers with explicit version #3196

feelepxyz · 2021-02-25T16:41:54Z

This runs existing bundler 1 native helpers by explicitly setting a BUNDLER_VERSION env var as well as pointing the BUNDLE_GEMFILE to bundler/helpers/v1/Gemfile which has been installed with BUNDLER_VERSION=1 bundle install setting BUNDLED WITH to the latest installed v1 version.

I've verified this setup works with bundler 2 installed in the core container and installing core's gems with v2 and running helpers with v1, I've got the native helper tests working with a setup that extends on this PR: https://github.com/dependabot/dependabot-core/pull/3211/files

A follow on ship from this would be running native helper specs with bundler 1 followed by actually installing bundler 2 and installing core's gems with it.

This also moves the native helpers to v1 folder making room for a v2 folder.

Release plan

Deploy to staging and verify a couple of different updates, including private registry/repo

Dockerfile

bundler/helpers/Gemfile.lock

bundler/helpers/Gemfile

feelepxyz · 2021-02-26T17:28:50Z

bundler/lib/dependabot/bundler/native_helpers.rb

+      end
+
+      def self.helper_path(bundler_version: "v1")
+        "ruby #{File.join(native_helpers_root, bundler_version, 'run.rb')}"


@jurre running without bundle exec worked and removed the need for an empty Gemfile 🎉

feelepxyz · 2021-02-26T17:30:09Z

bundler/helpers/v1/build

@@ -8,6 +8,10 @@ if [ -z "$install_dir" ]; then
  exit 1
 fi

+if [ ! -d "$install_dir" ]; then
+  mkdir -p "$install_dir"


creates the nested v1 in /opt/bundler

feelepxyz · 2021-02-26T17:31:07Z

bundler/lib/dependabot/bundler/native_helpers.rb

-      def self.helper_path
-        "bundle exec ruby #{File.join(native_helpers_root, 'run.rb')}"
+      def self.run_bundler_subprocess(function:, args:, bundler_version:)
+        bundler_env_version = bundler_version.tr("v", "")


This feels a bit funky, might be easier to do the version detection here instead

feelepxyz · 2021-02-26T17:33:33Z

@jurre @brrygrdn got specs passing with just running the native helpers with a re-set ENV and setting BUNDLER_VERSION, this worked with bundler 2 installed but the native helper specs broke so thinking it makes more sense to ship this incrementally and then follow up with a PR to install bundler 2 but not use and fix the native helper specs.

brrygrdn

Thanks for breaking this change out, it's very easy to follow the restructuring 👍🏻

I only have a couple of questions

brrygrdn · 2021-03-01T10:46:27Z

bundler/lib/dependabot/bundler/helpers.rb

+      V2 = "2"
+
+      # TODO: Add support for bundler v2
+      # return "v2" if lockfile.content.match?(/BUNDLED WITH\s+2/m)


brrygrdn · 2021-03-01T10:49:08Z

bundler/lib/dependabot/bundler/native_helpers.rb

+
+      def self.helper_path(bundler_version:)
+        native_helper_version = "v#{bundler_version}"
+        "ruby #{File.join(native_helpers_root, native_helper_version, 'run.rb')}"


Just to confirm, we've removed bundle exec intentionally in addition to clearing the env?

👍 intentionally removing as bundle exec needs to be run in a folder with a Gemfile source (or point to one with a env var) that has resolved dependencies, we currently don't rely on any dependencies in the native helpers so this just got in the way of running the native helpers with a custom bundler version. Bundler refuses to run BUNDLER_VERSION=1 cmd if the Gemfile.lock that we're pointing to has been resolved with BUNDLED WITH 2.x and vice versa.

Previously, when we executed bundle exec cmd it inherited all existing ENV values so ended up pointing to ./bundler/Gemfile as we ran the specs with bundle exec rspec spec and inherited the same BUNDLER_VERSION ENV var which I couldn't re-set without setting unsetenv_others: true.

Similarly, when run from the updater it would inherit the BUNDLED WITH from the updaters Gemfile.lock: https://github.com/dependabot/dependabot-updater/blob/main/bin/run#L21

common/lib/dependabot/shared_helpers.rb

brrygrdn · 2021-03-01T10:59:47Z

common/lib/dependabot/shared_helpers.rb

      start = Time.now
      stdin_data = JSON.dump(function: function, args: args)
      cmd = allow_unsafe_shell_command ? command : escape_command(command)
      env_cmd = [env, cmd].compact
      if ENV["DEBUG_FUNCTION"] == function
        escaped_stdin_data = stdin_data.gsub("\"", "\\\"")
-        puts "$ cd #{Dir.pwd} && echo \"#{escaped_stdin_data}\" | #{env_cmd.join(' ')}"
+        env_keys = env ? env.map { |k, v| "#{k}=#{v}" } : ""
+        puts "$ cd #{Dir.pwd} && echo \"#{escaped_stdin_data}\" | #{env_keys} #{cmd}"


I don't fully follow the switch from #{env_cmd.join(' ')} to #{env_keys} #{cmd} here, as it looks like we still pass env_cmd on line 89.

My read is that this purely to improve presentation in the logs, but I just wanted to check.

This path should only be used when you pass DEBUG_FUNCTION=somefunc as env var when running specs/dry-run and puts out the command that's being executed by ruby so you can cd into the temp dir and run the same thing in bash. The previous way of concatenating env and cmd didn't work when actually passing in a env hash.

The env_cmd arg still works for Open3 but now the bash version flattens the env hash to ENV_KEY=value ./func.

I attempted to clarify this a bit with the above comment and moving env_cmd down to where it's used

bundler/lib/dependabot/bundler/native_helpers.rb

feelepxyz · 2021-03-02T10:47:52Z

common/lib/dependabot/shared_helpers.rb

      if ENV["DEBUG_FUNCTION"] == function
-        escaped_stdin_data = stdin_data.gsub("\"", "\\\"")
-        puts "$ cd #{Dir.pwd} && echo \"#{escaped_stdin_data}\" | #{env_cmd.join(' ')}"
+        puts helper_subprocess_bash_command(stdin_data: stdin_data, command: cmd, env: env)


@brrygrdn moved this into it's own method

Example output, if you run DEBUG_FUNCTION=parsed_gemspec bundle exec rspec spec/dependabot/bundler/file_parser_spec.rb:685 you'll see this in console:

$ cd /Users/feelepxyz/code/dependabot-core/bundler/tmp/dependabot_20210302-86366-1xt144c && \ echo "{\"function\":\"parsed_gemspec\",\"args\":{\"gemspec_name\":\"example.gemspec\",\"lockfile_name\":null,\"dir\":\"/Users/feelepxyz/code/dependabot-core/bundler/tmp/dependabot_20210302-86366-1xt144c\"}}" \ | BUNDLER_VERSION=1 \ BUNDLE_GEMFILE=/Users/feelepxyz/code/dependabot-core/bundler/lib/dependabot/bundler/../../../helpers/v1/Gemfile \ bundle exec ruby /Users/feelepxyz/code/dependabot-core/bundler/lib/dependabot/bundler/../../../helpers/v1/run.rb

Nice, thanks for clarifying this 🙇🏻

This runs existing bundler 1 native helpers by explicitly setting a `BUNDLER_VERSION` env var as well as pointing the `BUNDLE_GEMFILE` to the `Gemfile` in `helpers/v1` which has been bundled with: `BUNDLER_VERSION=1 bundle install` setting `BUNDLED WITH` to the matching installed v1 version.

feelepxyz · 2021-03-02T11:04:08Z

bundler/lib/dependabot/bundler/native_helpers.rb

+            "RUBYLIB" => nil,
+            "RUBYOPT" => nil,
+            "GEM_PATH" => nil,
+            "GEM_HOME" => nil


This is kinda funky but seems to work, verified that these env vars are not set in the container so get set by bundler/ruby when running core with bundle exec rspec/updater run

brrygrdn

A couple of copy edits, otherwise this looks 💯

I don't feel bad unsetting specific ruby/bundler envvars given the circumstances. It's well called out and doesn't feel brittle in the sense those are pretty fixed points in the ecosystem.

brrygrdn · 2021-03-02T11:03:02Z

bundler/helpers/v1/Gemfile

+# gemspec so we've added it here for backwards compatability during bundler 2
+# rollout.
+#
+# NOTE: If we don't require it and a customers `.gemspec` uses File without


Suggested change

# NOTE: If we don't require it and a customers `.gemspec` uses File without

# NOTE: If we don't require it and a customers `.gemspec` uses Find without

brrygrdn · 2021-03-02T11:03:39Z

bundler/helpers/v1/Gemfile

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+# TODO: Look into removing this. "file" used to get required from common's


Suggested change

# TODO: Look into removing this. "file" used to get required from common's

# TODO: Look into removing this. "find" used to get required from common's

brrygrdn · 2021-03-02T11:06:37Z

bundler/lib/dependabot/bundler/native_helpers.rb

+            "RUBYLIB" => nil,
+            "RUBYOPT" => nil,
+            "GEM_PATH" => nil,
+            "GEM_HOME" => nil


This seems like a fair workaround, it feels like the cost of doing business since we're running bundler to run core to run bundler ♻️ 😅

brrygrdn · 2021-03-02T11:07:37Z

common/lib/dependabot/shared_helpers.rb

      if ENV["DEBUG_FUNCTION"] == function
-        escaped_stdin_data = stdin_data.gsub("\"", "\\\"")
-        puts "$ cd #{Dir.pwd} && echo \"#{escaped_stdin_data}\" | #{env_cmd.join(' ')}"
+        puts helper_subprocess_bash_command(stdin_data: stdin_data, command: cmd, env: env)


Nice, thanks for clarifying this 🙇🏻

brrygrdn

🚀

…pike" This reverts commit a0dd603, reversing changes made to c93e297.

feelepxyz requested a review from a team as a code owner February 25, 2021 16:41

feelepxyz commented Feb 26, 2021

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

feelepxyz commented Feb 26, 2021

View reviewed changes

bundler/helpers/Gemfile.lock Outdated Show resolved Hide resolved

feelepxyz commented Feb 26, 2021

View reviewed changes

bundler/helpers/Gemfile Outdated Show resolved Hide resolved

feelepxyz changed the title ~~WIP: Bundler 2~~ Run updates with bundler 1 with bundler 2 installed Feb 26, 2021

feelepxyz changed the title ~~Run updates with bundler 1 with bundler 2 installed~~ Run bundler v1 native helpers with unset ENV Feb 26, 2021

feelepxyz changed the title ~~Run bundler v1 native helpers with unset ENV~~ Run Bundler v1 native helpers with unset ENV Feb 26, 2021

feelepxyz commented Feb 26, 2021

View reviewed changes

feelepxyz requested a review from brrygrdn February 26, 2021 17:29

feelepxyz commented Feb 26, 2021

View reviewed changes

brrygrdn reviewed Mar 1, 2021

View reviewed changes

feelepxyz force-pushed the feelepxyz/bundler2-spike branch from ec8f2ed to e339e4c Compare March 1, 2021 14:13

feelepxyz commented Mar 1, 2021

View reviewed changes

bundler/lib/dependabot/bundler/native_helpers.rb Outdated Show resolved Hide resolved

Rename bundler v1 helpers

191cb50

feelepxyz force-pushed the feelepxyz/bundler2-spike branch from fc98076 to 5e3da4c Compare March 2, 2021 10:34

feelepxyz changed the title ~~Run Bundler v1 native helpers with unset ENV~~ Run Bundler v1 native helpers with explicit version Mar 2, 2021

feelepxyz force-pushed the feelepxyz/bundler2-spike branch from 5e3da4c to 1612bcd Compare March 2, 2021 10:45

feelepxyz commented Mar 2, 2021

View reviewed changes

feelepxyz force-pushed the feelepxyz/bundler2-spike branch from 1612bcd to b088744 Compare March 2, 2021 10:53

feelepxyz requested a review from brrygrdn March 2, 2021 10:58

feelepxyz commented Mar 2, 2021

View reviewed changes

brrygrdn reviewed Mar 2, 2021

View reviewed changes

Fix comment typos

2cfc8e3

brrygrdn approved these changes Mar 2, 2021

View reviewed changes

feelepxyz merged commit a0dd603 into main Mar 2, 2021

feelepxyz deleted the feelepxyz/bundler2-spike branch March 2, 2021 11:18

feelepxyz added a commit that referenced this pull request Mar 3, 2021

Revert "Merge pull request #3196 from dependabot/feelepxyz/bundler2-s…

b8ec905

…pike" This reverts commit a0dd603, reversing changes made to c93e297.

feelepxyz mentioned this pull request Mar 3, 2021

Revert: Run Bundler v1 native helpers with explicit version #3222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run Bundler v1 native helpers with explicit version #3196

Run Bundler v1 native helpers with explicit version #3196

feelepxyz commented Feb 25, 2021 •

edited

Loading

feelepxyz Feb 26, 2021

feelepxyz Feb 26, 2021

feelepxyz Feb 26, 2021

feelepxyz commented Feb 26, 2021

brrygrdn left a comment

brrygrdn Mar 1, 2021

brrygrdn Mar 1, 2021

feelepxyz Mar 1, 2021

feelepxyz Mar 1, 2021

brrygrdn Mar 1, 2021

feelepxyz Mar 1, 2021

feelepxyz Mar 1, 2021

feelepxyz Mar 2, 2021 •

edited

Loading

brrygrdn Mar 2, 2021

feelepxyz Mar 2, 2021

brrygrdn left a comment

brrygrdn Mar 2, 2021

brrygrdn Mar 2, 2021

brrygrdn Mar 2, 2021

brrygrdn Mar 2, 2021

brrygrdn left a comment

	# NOTE: If we don't require it and a customers `.gemspec` uses File without
	# NOTE: If we don't require it and a customers `.gemspec` uses Find without

		@@ -0,0 +1,16 @@
		# frozen_string_literal: true

		# TODO: Look into removing this. "file" used to get required from common's

	# TODO: Look into removing this. "file" used to get required from common's
	# TODO: Look into removing this. "find" used to get required from common's

Run Bundler v1 native helpers with explicit version #3196

Run Bundler v1 native helpers with explicit version #3196

Conversation

feelepxyz commented Feb 25, 2021 • edited Loading

Release plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feelepxyz commented Feb 26, 2021

brrygrdn 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

feelepxyz Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brrygrdn 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

brrygrdn left a comment

Choose a reason for hiding this comment

feelepxyz commented Feb 25, 2021 •

edited

Loading

feelepxyz Mar 2, 2021 •

edited

Loading