-
Notifications
You must be signed in to change notification settings - Fork 192
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 #5181 - when using --bundle in the CLI, use Embedded native gems when they are present and version compatible #5182
Conversation
…when they are present and version compatible
# Global list, to be populated below | ||
$ignore_native_gem_names = [] | ||
|
||
module Bundler | ||
class RubygemsIntegration | ||
|
||
alias :ori_spec_missing_extensions? :spec_missing_extensions? | ||
|
||
def spec_missing_extensions?(spec, default = true) | ||
|
||
# This avoids getting an annoying message for no reason | ||
if $ignore_native_gem_names.any? {|name| name == spec.name } | ||
return false | ||
end | ||
|
||
return ori_spec_missing_extensions?(spec, default) | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define a global variable that will store the gem names for which we do have a compatible native extension inside the CLI.
And patch spec_missing_extensions? to just shush the warning that will remain anyways
# Filter out dependencies with native extensions that we already have embedded in the CLI and that satisfy the requirements | ||
locked_specs = Bundler.definition.instance_variable_get(:@locked_specs) | ||
|
||
embedded_gems_to_activate.each do |spec| | ||
next if spec.extensions.empty? | ||
|
||
next unless locked_specs.any? {|locked_spec| locked_spec.name == spec.name} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, main dish starts here. Apologies in advance for what you're going to witness...
Step 1, grab the locked_specs, which is a private instance variable, and is the key one that bundler will use to resolve.
We start by looping on the emedded_gems_to_activate, and we keep only:
- Gems with native extensions
- Gems actually used by bundler as well.
dep_to_requirements = {} | ||
locked_specs.each do |locked_spec| | ||
deleted_deps = locked_spec.dependencies.select { |dep| dep.name == spec.name && dep.requirement.satisfied_by?(spec.version) } | ||
locked_spec.dependencies.reject! { |dep| dep.name == spec.name && dep.requirement.satisfied_by?(spec.version) } | ||
deleted_deps&.each do |dep| | ||
dep_to_requirements[locked_spec.name] = dep.requirement.to_s | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass one, we loop on all of these locked_specs, and we can their dependencies to try to see if they meet our current spec or not. If so, delete it (and store the requirements in a hash for reporting purposes).
For reference, spec (from embedded_gems_to_activate) looks like
Gem::Specification.new do |s|
s.name = "json"
s.version = Gem::Version.new("2.7.2")
...
locked_spec looks like this
locked_spec
=>
#<Bundler::LazySpecification:0x00007f52209429f0
@dependencies=
[Gem::Dependency.new("json", Gem::Requirement.new(["~> 2.3"]), :runtime),
Gem::Dependency.new("parallel", Gem::Requirement.new(["~> 1.10"]), :runtime),
Gem::Dependency.new("parser", Gem::Requirement.new([">= 3.2.0.0"]), :runtime),
Gem::Dependency.new("rainbow", Gem::Requirement.new([">= 2.2.2", "< 4.0"]), :runtime),
Gem::Dependency.new("regexp_parser", Gem::Requirement.new([">= 1.8", "< 3.0"]), :runtime),
Gem::Dependency.new("rexml", Gem::Requirement.new([">= 3.2.5", "< 4.0"]), :runtime),
Gem::Dependency.new("rubocop-ast", Gem::Requirement.new([">= 1.28.0", "< 2.0"]), :runtime),
Gem::Dependency.new("ruby-progressbar", Gem::Requirement.new(["~> 1.7"]), :runtime),
Gem::Dependency.new("unicode-display_width", Gem::Requirement.new([">= 2.4.0", "< 3.0"]), :runtime)],
@force_ruby_platform=false,
@full_name="rubocop-1.52.0",
@name="rubocop",
@platform="ruby",
@source=#<Bundler::Source::Rubygems:0x30140 locally installed gems>,
@use_exact_resolved_specifications=true,
@version=Gem::Version.new("1.52.0")>
if locked_specs.none? { |locked_spec| locked_spec.dependencies.any? {|x| x.name == spec.name} } | ||
locked_spec_v = locked_specs.select{|locked_spec| locked_spec.name == spec.name}.first.version | ||
$logger.debug("Removing native gem #{spec.name} from Bundler locked_specs (version #{locked_spec_v}), using the embedded dependency (CLI one has version #{spec.version})") | ||
$logger.trace("Gems having #{spec.name} as a dependency: #{dep_to_requirements}") | ||
raise "Failed to activate #{spec.name}" unless spec.activate | ||
$ignore_native_gem_names << spec.name | ||
locked_specs.delete_by_name(spec.name) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If after cleaning all dependencies, there are none left, then we actually remove the locked_spec called "spec.name" itself.
So for eg,
locked_specs has originally one for rubocop, with a dependency on json. We remove this one. And it was the only one.
Then we actually delete the locked_spec named json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a result, when you run with Trace loglevel, you see
[ruby] <-1> Bundling with groups [default,development]
[ruby] <-2> Removing native gem json from Bundler locked_specs (version 2.7.2), using the embedded dependency (CLI one has version 2.6.3)
[ruby] <-3> Gems having json as a dependency: {"rubocop"=>"~> 2.3"}
Found no changes, using resolution from the lockfile
[ruby] <-1> Specs to be included [rake,public_suffix,addressable,ansi,ast,builder,multipart-post,faraday,minitar,rchardet,git,minitest,ruby-progressbar,minitest-reporters,parser,parallel,rainbow,regexp_parser,rexml,rubocop-ast,unicode-display_width,rubocop,rubocop-checkstyle_formatter,rubocop-performance,docile,simplecov-html,simplecov,openstudio_measure_tester,rubyzip,ruby-ole,spreadsheet,systemu,macaddr,uuid,yamler,zliby,bcl,bundler,diff-lcs,json-schema,matrix,sawyer,octokit,openstudio-ee,openstudio-extension,oslg,osut,topolys,tbd,openstudio-standards,openstudio-workflow,rspec-support,rspec-core,rspec-expectations,rspec-mocks,rspec]
Specs to be included no longer contains json.
locked_spec_v = locked_specs.select{|locked_spec| locked_spec.name == spec.name}.first.version | ||
$logger.debug("Removing native gem #{spec.name} from Bundler locked_specs (version #{locked_spec_v}), using the embedded dependency (CLI one has version #{spec.version})") | ||
$logger.trace("Gems having #{spec.name} as a dependency: #{dep_to_requirements}") | ||
raise "Failed to activate #{spec.name}" unless spec.activate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is even needed to try and activate now, but I find it logical. Maybe I should have also deleted it from embedded_gems_to_activate, not sure.
# clean up the global | ||
$ignore_native_gem_names = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the end I clean up the global, because I'm a good citizen.
In ruby $<whatever>
returns nil anyways if not defined.
I think we should also do the same for our $logger
global variable, but not the point there.
Note that if I put directly `gem 'json', '~> 2.3'` instead of going indirectly through rubocop, then it fails... so not perfect. @kbenne FYI
CI Results for 41c212b:
|
Pull request overview
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.