-
Notifications
You must be signed in to change notification settings - Fork 231
Fix #5406 - Regression in CLI on Windows: openstudio ruby_version fails on develop #5407
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
Changes from all commits
264de9d
a32143d
e29db41
04dd5f1
400c30a
4bf26cb
723ca8d
6667e19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ def self.get_absolute_path(p) | |
| end | ||
| absolute_path = ':' + absolute_path | ||
| else | ||
| absolute_path = File.expand_path p2 | ||
| absolute_path = File.expand_path p | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weirdly enough, this has always been broken. p2 isn't defined here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so we never called this function on a non embedded file?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct. We don't use it directly basically, and checks were doing prior to calling it. |
||
| end | ||
| return absolute_path | ||
| end | ||
|
|
@@ -626,8 +626,10 @@ def self.[](*args) | |
| end | ||
| end | ||
|
|
||
| def self.glob(pattern, *args, **options) | ||
| def self.glob(pattern, _flags = 0, flags: _flags, base: nil, sort: true) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, so strap yourself. First off: In Ruby 3.x, especially post-3.1, many core methods have been migrated to use the new RubyVM Primitive interface — which acts as a bridge between Ruby and C-defined functions, often exposed via FFI-like syntax for performance and modularity. https://github.com/ruby/ruby/blob/f91480d7a671b1b114270a4b5e4d3c5aa6dabce9/dir.rb#L219-L221 def self.glob(pattern, _flags = 0, flags: _flags, base: nil, sort: true)
Primitive.dir_s_glob(pattern, flags, base, sort)
end
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Side note about what the heck that this:
This is subtle Ruby trick used for backward compatibility with older versions of Ruby where In other words:
|
||
|
|
||
| debug = false | ||
| # debug = !base.nil? && base.start_with?(':/ruby/gems/3.2.0/specifications/default') | ||
| pattern_array = [] | ||
| if pattern.is_a? String | ||
| pattern_array = [pattern] | ||
|
|
@@ -637,45 +639,71 @@ def self.glob(pattern, *args, **options) | |
| pattern_array = pattern | ||
| end | ||
|
|
||
| #puts "Dir.glob pattern = #{pattern}, pattern_array = #{pattern_array}, args = #{args}, options = #{options}" | ||
| override_args_extglob = false | ||
|
|
||
| result = [] | ||
| pattern_array.each do |pattern| | ||
| pattern_has_embedded = pattern_array.any? {|p| p.to_s.chars.first == ':'} | ||
| base_has_embedded = (!base.nil? && base.to_s.chars.first == ':') | ||
| if !pattern_has_embedded && !base_has_embedded | ||
| # puts "Original glob" | ||
| return self.original_glob(pattern, flags: flags, base: base, sort: sort) | ||
| end | ||
|
Comment on lines
+642
to
+647
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If nothing has an embedded thingy, just forward to the original method. |
||
|
|
||
| if pattern.to_s.chars.first == ':' | ||
| absolute_base = if base.nil? | ||
| nil | ||
| else | ||
| OpenStudio.get_absolute_path(base) | ||
| end | ||
|
Comment on lines
+649
to
+653
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the base is provided, get_absolute_path it, which will just do File.expand_path base if base is not embedded. I could have used File.expand_path or File.absolute_path I guess, which we override. |
||
| if debug | ||
| puts "pattern_array=#{pattern_array}" | ||
| puts "base=#{base}" | ||
| puts "flags=#{flags}" | ||
| puts "pattern_has_embedded=#{pattern_has_embedded}" | ||
| puts "base_has_embedded=#{base_has_embedded}" | ||
| puts "absolute_base=#{absolute_base}" | ||
| end | ||
|
|
||
| # DLM: seems like this is needed for embedded paths, possibly due to leading ':' character? | ||
| override_args_extglob = true | ||
| # DLM: seems like this is needed for embedded paths, possibly due to leading ':' character? | ||
| # JM (2025): Seems like fnmatch behaves differently than Dir.glob | ||
| # fnmatch specifically needs EXTGLOB to allow patterns like '{a,b}' while | ||
| # glob seems to allow that directly | ||
| override_args_extglob = true | ||
|
|
||
| #puts "searching embedded files for #{pattern}" | ||
| absolute_pattern = OpenStudio.get_absolute_path(pattern) | ||
| #puts "absolute_pattern #{absolute_pattern}" | ||
| flags = flags | File::FNM_EXTGLOB if override_args_extglob | ||
| result = [] | ||
| pattern_array.each do |pattern| | ||
|
|
||
| EmbeddedScripting::fileNames.each do |name| | ||
| absolute_path = OpenStudio.get_absolute_path(name) | ||
| absolute_pattern = if pattern.to_s.chars.first == ':' | ||
| OpenStudio.get_absolute_path(pattern) | ||
| elsif !base.nil? | ||
| File.expand_path(pattern, base) | ||
| else | ||
| pattern | ||
| end | ||
| if debug | ||
| puts "searching embedded files for #{pattern}" | ||
| puts "absolute_pattern #{absolute_pattern}" | ||
| end | ||
|
|
||
| if override_args_extglob | ||
| if File.fnmatch( absolute_pattern, absolute_path, File::FNM_EXTGLOB ) | ||
| #puts "#{absolute_path} is a match!" | ||
| result << absolute_path | ||
| end | ||
| else | ||
| if File.fnmatch( absolute_pattern, absolute_path, *args, **options ) | ||
| #puts "#{absolute_path} is a match!" | ||
| result << absolute_path | ||
| end | ||
| EmbeddedScripting::fileNames.each do |name| | ||
| absolute_path = OpenStudio.get_absolute_path(name) | ||
| if base_has_embedded | ||
| next unless absolute_path.start_with?(absolute_base) | ||
| if debug | ||
| puts "name=#{name}, absolute_path=#{absolute_path}" | ||
| puts "absolute_path.start_with?(absolute_base)=#{absolute_path.start_with?(absolute_base)}" | ||
|
Comment on lines
+685
to
+691
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. loop on the embedded fileNames (which all start with ':/'), skip if they don't match the base |
||
| end | ||
|
|
||
| end | ||
|
|
||
| else | ||
| if override_args_extglob | ||
| result.concat(self.original_glob(pattern, File::FNM_EXTGLOB)) | ||
| else | ||
| result.concat(self.original_glob(pattern, *args, **options)) | ||
| #if override_args_extglob | ||
| # if File.fnmatch( absolute_pattern, absolute_path, File::FNM_EXTGLOB ) | ||
| if File.fnmatch(absolute_pattern, absolute_path, flags) | ||
| #puts "#{absolute_path} is a match!" | ||
| result << absolute_path | ||
|
Comment on lines
+695
to
+699
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can fnmatch, afaik there's no need to override the flags. |
||
| end | ||
| end | ||
|
|
||
| end | ||
|
|
||
| if debug | ||
| puts result | ||
| end | ||
|
|
||
| if block_given? | ||
|
|
||
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.
When LogLevel = Trace, don't pre-require IRB on windows, it fails
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.
Fixes #5216?
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.
Yeah thanks.
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.
Wait, so why no irb on windows? It fails due to no io/console or because of the logging bug?
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.
no io/console