Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Redo: Run Bundler v1 helpers with explicit version #3223

Merged
merged 1 commit into from
Mar 8, 2021
Merged

Redo: Run Bundler v1 helpers with explicit version #3223

merged 1 commit into from
Mar 8, 2021

Conversation

feelepxyz
Copy link
Contributor

This brings back this revert: #3222

Work in progress to figure out how to run multiple bundler version

@feelepxyz feelepxyz requested a review from a team as a code owner March 3, 2021 12:18
@feelepxyz feelepxyz changed the title Redo: Run Bundler v1 helpers with explicit version … Redo: Run Bundler v1 helpers with explicit version Mar 3, 2021
@brrygrdn
Copy link
Contributor

brrygrdn commented Mar 4, 2021 •
edited
Loading

Test failures aside, I think using Bundler's own environment management is a step in the right direction, I've been able to get a sensible outcome across the three test repos along with our specific git-dependency test https://github.com/dsp-testing/bundler1-git-test

bin/dry-run.rb bundler dsp-testing/bundler1-git-test 
=== byebug (fead41cb53d349e11045a05cc804740089dbfa80)
 => checking for updates 1/2
 => latest available version is 11.1.3
 => latest allowed version is 11.1.3
 => requirements to unlock: own
 => requirements update strategy: bump_versions
 => updating byebug from fead41cb53d349e11045a05cc804740089dbfa80 to 11.1.3
    ± Gemfile
    ~~~
    3c3
    < gem "byebug", git: "git@github.com:deivid-rodriguez/byebug.git", ref: "v11.1.0"
    ---
    > gem "byebug"
    ~~~
    ± Gemfile.lock
    ~~~
    1,7d0
    < GIT
    <   remote: git@github.com:deivid-rodriguez/byebug.git
    <   revision: fead41cb53d349e11045a05cc804740089dbfa80
    <   ref: v11.1.0
    <   specs:
    <     byebug (11.1.0)
    <
    11a5
    >     byebug (11.1.3)
    18c12
    <   byebug!
    ---
    >   byebug
    ~~~

@brrygrdn brrygrdn self-assigned this Mar 4, 2021
feelepxyz
feelepxyz commented Mar 4, 2021
View reviewed changes
bundler/lib/dependabot/bundler/native_helpers.rb
# Bundler will pick the matching installed major version
"BUNDLER_VERSION" => bundler_version,
"BUNDLE_GEMFILE" => File.join(versioned_helper_path(bundler_version: bundler_version), "Gemfile"),
"BUNDLE_PATH" => File.join(versioned_helper_path(bundler_version: bundler_version), ".bundle")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to match BUNDLE_GEMFILE but not sure it's necessary, both seemed to work inside and outside the container with v1 and v2 installed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed synchronously, I'm 👍🏻 on having versioned_helper_path(bundler_version: bundler_version) as a clean(ish) room bundler path for all things related to the native helper process.

brrygrdn
brrygrdn reviewed Mar 5, 2021
View reviewed changes
Copy link
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending some further testing, I think this is ready to 🚢 🎉

Since it took a lot of iterations to get this working end to end, how do you feel about squashing the commit history for merge?

@feelepxyz feelepxyz force-pushed the feelepxyz/revert-revert-pull-3196 branch 2 times, most recently from 7a07768 to d38670d Compare March 5, 2021 15:40
@feelepxyz feelepxyz requested a review from brrygrdn March 5, 2021 15:40
brrygrdn
brrygrdn approved these changes Mar 5, 2021
View reviewed changes
Copy link
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

feelepxyz
feelepxyz commented Mar 5, 2021
View reviewed changes
bundler/lib/dependabot/bundler/native_helpers.rb
env: {
# Bundler will pick the matching installed major version
"BUNDLER_VERSION" => bundler_version,
"BUNDLE_GEMFILE" => File.join(versioned_helper_path(bundler_version: bundler_version), "Gemfile"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required when running with bundler 2 installed alongside v1, without this it defaults to v2 during dry-runs.

brrygrdn
brrygrdn reviewed Mar 5, 2021
View reviewed changes
bundler/spec/fixtures/projects/bundler1/gemspec_with_require_no_lockfile/example.gemspec
@@ -1,5 +1,7 @@
# frozen_string_literal: true

require "find"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed synchronously, the use of Find in these gemspecs is atypical and not suggested by Bundler v1.17.x or v2.x.x when generating new gems.

Since Find isn't in the ruby context by default, we'd expect .gemspecs in the real world to have this require

@feelepxyz @brrygrdn
Redo: Run Bundler v1 helpers with explicit version
1535a20 
This brings back this revert:
#3222

We've wrapped bundler native helpers in `Bundler.with_original_env` to
use the env before bundler was initialised in core.

This removed the need to manually unset env vars like `GEM_HOME` and
`GEM_PATH` which caused bundler to default git install to
`/var/lib/gems/2.6.0/cache/bundler/git` when run in the parent
container, where it doesn't have write permission.

With this change the the bundle cache is written to the native helper
folder/.bundle.

Co-authored-by: Barry Gordon <brrygrdn@github.com>
@feelepxyz feelepxyz force-pushed the feelepxyz/revert-revert-pull-3196 branch from 5b0a34b to 1535a20 Compare March 5, 2021 16:15
@feelepxyz feelepxyz merged commit 49d179e into main Mar 8, 2021
@feelepxyz feelepxyz deleted the feelepxyz/revert-revert-pull-3196 branch March 8, 2021 09:16
This was referenced Mar 9, 2021
Closed
Closed
This was referenced Mar 16, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Mar 17, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brrygrdn brrygrdn brrygrdn approved these changes

Assignees

@brrygrdn brrygrdn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@feelepxyz @brrygrdn