Use #is_a_path? conditionally on Bundler version #19
Conversation
|
Actually -- This appears to fix a broken ComponentTree test in CI on the pr build. So there is some proof of concept. |
garettarrowood
force-pushed
the
bugfix/bundler_version
branch
from
722cc02
to
79d8588
Compare
| end | ||
|
|
||
| def bundler_minor_version | ||
| bundler_definition.locked_bundler_version.split(".")[1].to_i |
There was a problem hiding this comment.
Do you care about the locked version, or the version in use at runtime?
There was a problem hiding this comment.
The one in use at runtime.
Good catch with that method name.
It appears though, that this actually returns the runtime version. As the locked version in this repo was 1.16.0 for me, but my local version was 1.16.3.
Given how much Bundler's api appears to change, what about something like:
stout, _ = Open3.capture2("bundler", "-v")More reliable, or hacky?
There was a problem hiding this comment.
Less reliable: the version in use and the version in the path might potentially be different.
Is Bundler::VERSION not defined? If not, I would ask rubygems for the version of Bundler that has been loaded.
There was a problem hiding this comment.
Bundler::VERSION is accessible :).
garettarrowood
force-pushed
the
bugfix/bundler_version
branch
from
3858d9e
to
b6caad9
Compare
garettarrowood
force-pushed
the
bugfix/bundler_version
branch
from
f9b3ed0
to
03c5439
Compare
|
@benlangfeld - Ready for review. The last PR build actually passed. But travis failed to retrieve libraries in a couple spots. You can see from the large amount of commits that I had trouble getting travis-ci to use the Bundler version. What we had setup in the Good news. Bundler API's On a whim I added some Ruby versions to the new travis matrix. And currently there are 4 minor versions of Bundler specified. All together, that means 12 separate builds for both |
.travis.yml
Outdated
| - sudo apt-get -qq install graphviz | ||
| rvm: | ||
| - 2.3.7 |
There was a problem hiding this comment.
I think we can be aggressive and state support only for Ruby > 2.5.0
.travis.yml
Outdated
| env: | ||
| - BUNDLER_VERSION=1.13.6 | ||
| - BUNDLER_VERSION=1.14.6 | ||
| - BUNDLER_VERSION=1.15.4 |
There was a problem hiding this comment.
Equally, we can probably ignore Bundler < 1.15.0.
| before_install: | ||
| - gem install bundler -v 1.13.6 | ||
| - "find /home/travis/.rvm/rubies -wholename '*default/bundler-*.gemspec' -delete" |
There was a problem hiding this comment.
Does gem uninstall not work?
There was a problem hiding this comment.
Unfortunately not. I tried in the before_install and install phases. Travis returns:
$ gem uninstall bundler
ERROR: While executing gem ... (Gem::InstallError)
gem "bundler" cannot be uninstalled because it is a default gem
| @@ -93,6 +99,14 @@ def gemfile_path | |||
| def gemfile_lock_path | |||
| File.join(@root_path, "Gemfile.lock") | |||
| end | |||
|
|
|||
| def bundler_version_supporting_path_method? | |||
| @bundler_version_supporting_path_method ||= bundler_minor_version >= 16 | |||
There was a problem hiding this comment.
I would actually do a comparison on the whole version. There’s a helper method somewhere in Rubygems (maybe Gem::Version comparison operator) to do that more accurately than ignoring the major version.
| format(gems) | ||
| end | ||
| end | ||
|
|
||
| def path?(dep) | ||
| if bundler_version_supporting_path_method? |
There was a problem hiding this comment.
Thinking about it, I wonder if we shouldn’t just rely on responds_to? here?
| def path?(source) | ||
| return if source.nil? | ||
| if source.respond_to?(:path?) | ||
| source.path? && source.path.to_s != "." |
There was a problem hiding this comment.
Last nit: any chance of reducing the duplication here?
There was a problem hiding this comment.
I'll see what I can do. I was thinking that if source does respond to path?, it may still return falsy. And if it does return falsy, then it may not respond to path (without the ?).
Needs a test still.
Any comments on this implementation?