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.

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

Fix incorrect re-resolution when path gem excluded and not available #3902

Merged
merged 1 commit into from Aug 30, 2020

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Aug 25, 2020

Description:

According to the debugging messages, if specs for a path gem are not available at bundler/setup time, but the group the gem belongs to has been excluded, it should be assumed that the path source has no changes and continue.

However, there's another check for re-resolution that checks completeness for the current platform that was not filtering for requested groups only, and making this case to incorrectly re-resolve, and as a result fail due to the path gem not being available (even if not requested).

This commit should fix the problem.

Fixes #3177.

Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

According to the debugging messages, if specs for a path gem are not
available at `bundler/setup` time, but the group the gem belongs to has
been excluded, it should be assume that the path source has no changes
and continue.

However, there's another check for re-resolution that checks
completeness for the current platform that was not filtering for
requested groups only, and making this case to incorrectly re-resolve,
and as a result fail due to the path gem not being available (even if
not requested).

This commit should fix the problem.
@fzakaria
Copy link

Wow that was the fix ! So small 👍
I tried to introspect bundler myself but to be honest the code was a bit confusing for me to navigate.

I'm on paternity now; but I will try to test out the fix by EOW :)

@deivid-rodriguez
Copy link
Member Author

I know, bundler's code gets superconfusing. Your investigation in the original ticket was very helpful and on point, and made it much easier to figure this out. Still let's wait for the CI and your tests, hopefully this is correct.

@deivid-rodriguez
Copy link
Member Author

I'm on paternity now;

Congratulations, by the way 😄

@fzakaria
Copy link

thanks!
Yes I try when possible at least to give as much information.
Glad it helped;

Our codebase is pretty old & large so we do quite a lot of "interesting" Ruby/JRuby/Bundler/Warbler patterns.

@deivid-rodriguez
Copy link
Member Author

@fzakaria Did you find time to try this? No rush at all, I was just wondering, since I looked for the meaning of the EOW acronym and it means End of Week 😄

@fzakaria
Copy link

Yea ; sorry for the late reply! the baby is making things tough to follow-through on 👍

I tested it in fact with my minimal reproduction project.
https://github.com/fzakaria/warbler-bundler-jruby-failure

Here are the steps:

  1. Confirm failure; using bundler 2.0.2 (separate issue affecting 2.1.4)
ruby '2.5.7', engine: 'jruby', engine_version: '9.2.13.0'
source 'https://rubygems.org'

group :development, :test do
   gem 'demo', :path => "./ruby-gems/demo"
end
gem 'warbler', '2.0.5'
gem 'jruby-jars', '9.2.13.0'
$ bundle exec warble
$ java -jar warbler-bundler-jruby-failure.jar

The path `uri:classloader://warbler-bundler-jruby-failure/ruby-gems/demo` does not exist.
  1. Checkout your pull-request and build the gem
# remove all previous bundlers
gem uninstall bundler
gem build bundler.gemspec`
gem install bundler-2.2.0.rc.2.gem 
  1. Profit
$ bundle exec warble
$ java -jar warbler-bundler-jruby-failure.jar

Starting the JAR!

PS: is there a more fancier way to test developing gems ? Python has the ability to perform a "dev install" which symlinks to the directory rather than building a full gem.

@deivid-rodriguez
Copy link
Member Author

Great, thanks so much for double checking even if it's hard now with the baby! ;)

PS: is there a more fancier way to test developing gems ? Python has the ability to perform a "dev install" which symlinks to the directory rather than building a full gem.

With other gems, you would normally use the path: option in the Gemfile and don't need to do anything else, but for bundler itself, we need to do extra magic like what you did. I normally run bin/rake install, which builds and installs in one step.

@deivid-rodriguez
Copy link
Member Author

Merging since @fzakaria has confirmed this works and the PR received no other feedback.

@deivid-rodriguez deivid-rodriguez merged commit dfa7b0b into master Aug 30, 2020
@deivid-rodriguez deivid-rodriguez deleted the fix/wrong_platform_check_for_reresolving branch August 30, 2020 21:25
deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020
…eresolving

Fix incorrect re-resolution when path gem excluded and not available

(cherry picked from commit dfa7b0b)
deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020
…eresolving

Fix incorrect re-resolution when path gem excluded and not available

(cherry picked from commit dfa7b0b)
deivid-rodriguez added a commit that referenced this pull request Oct 5, 2020
…eresolving

Fix incorrect re-resolution when path gem excluded and not available

(cherry picked from commit dfa7b0b)
deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020
…eresolving

Fix incorrect re-resolution when path gem excluded and not available

(cherry picked from commit dfa7b0b)
deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020
…eresolving

Fix incorrect re-resolution when path gem excluded and not available

(cherry picked from commit dfa7b0b)
deivid-rodriguez added a commit that referenced this pull request Oct 6, 2020
…eresolving

Fix incorrect re-resolution when path gem excluded and not available

(cherry picked from commit dfa7b0b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bundler requires all :path gems to be in default group
3 participants