Fix #5406 - Regression in CLI on Windows: openstudio ruby_version fails on develop #5407

jmarrec · 2025-05-14T07:59:21Z

Pull request overview

Fixes Regression in CLI on Windows: openstudio ruby_version fails on develop #5406
Related to Fix #5388 - --bundle options not working for CLI in docker-openstudio, possibly OS itself + Fix #5190 - Don't pick up system gem in CLI #5394
Fixes --loglevel Trace fails on Windows #5216

More ruby shenanigans due to embedded filesystem! Yay!

Eventually I traced down the issue.

rubygems.rb calls Gem::Specification.load_defaults
- https://github.com/ruby/ruby/blob/f91480d7a671b1b114270a4b5e4d3c5aa6dabce9/lib/rubygems.rb#L1355
That uses each_spec([Gem.default_specifications_dir])
Gem.default_specifications_dir is now :/ruby/gems/3.2.0/specifications/default" because Fix #5388 - --bundle options not working for CLI in docker-openstudio, possibly OS itself + Fix #5190 - Don't pick up system gem in CLI #5394 changed the rubylibprefix and co to point to the embedded filesystem
That ends up calling rubygems/util.rb#glob_files_in_dir
- https://github.com/ruby/ruby/blob/f91480d7a671b1b114270a4b5e4d3c5aa6dabce9/lib/rubygems/util.rb#L100-L102
- Essentially calling Dir.glob('*.gemspec', ':/ruby/gems/3.2.0/specifications/default')
- ANNNND KABOOM 💥

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

jmarrec

Not particularly happy about how fragile this all seems, but it fixes the immediate issue.

@kbenne please review!

jmarrec · 2025-05-14T07:59:47Z

ruby/bindings/InitRubyBindings.cpp


    openstudio::evalString(initScript);

+#ifndef _WIN32  // no io/console available


When LogLevel = Trace, don't pre-require IRB on windows, it fails

Fixes #5216?

Yeah thanks.

Wait, so why no irb on windows? It fails due to no io/console or because of the logging bug?

no io/console

jmarrec · 2025-05-14T08:00:08Z

ruby/engine/embedded_help.rb

      absolute_path = ':' + absolute_path
    else
-      absolute_path = File.expand_path p2
+      absolute_path = File.expand_path p


Weirdly enough, this has always been broken. p2 isn't defined here.

so we never called this function on a non embedded file?

correct. We don't use it directly basically, and checks were doing prior to calling it.

jmarrec · 2025-05-14T08:05:41Z

ruby/engine/embedded_help.rb

  end

-  def self.glob(pattern, *args, **options)
+  def self.glob(pattern, _flags = 0, flags: _flags, base: nil, sort: true)


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

Side note about what the heck that this:

def self.glob(pattern, _flags = 0, flags: _flags, base: nil, sort: true)

This is subtle Ruby trick used for backward compatibility with older versions of Ruby where Dir.glob accepted a positional second argument (flags) instead of keyword arguments.

_flags = 0 # Default value if the second positional argument isn't given flags: _flags # Keyword argument `:flags` defaults to the positional `_flags`

In other words:

Calling Dir.glob("*.rb", File::FNM_DOTMATCH) (using the old 2-argument form), _flags will be assigned File::FNM_DOTMATCH, and flags: will default to that.

Calling Dir.glob("*.rb", flags: File::FNM_DOTMATCH), the keyword argument will override the default.

jmarrec · 2025-05-14T08:08:36Z

ruby/engine/embedded_help.rb

+  def self.glob(pattern, _flags = 0, flags: _flags, base: nil, sort: true)

+    debug = false
+    debug = !base.nil? && base.start_with?(':/ruby/gems/3.2.0/specifications/default')


I need to remove this

jmarrec · 2025-05-14T08:09:18Z

ruby/engine/embedded_help.rb

+    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


If nothing has an embedded thingy, just forward to the original method.

jmarrec · 2025-05-14T08:10:51Z

ruby/engine/embedded_help.rb

+    absolute_base = if base.nil?
+                      nil
+                    else
+                      OpenStudio.get_absolute_path(base)
+                    end


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.

jmarrec · 2025-05-14T08:13:20Z

ruby/engine/embedded_help.rb

+      absolute_pattern = if pattern.to_s.chars.first == ':'
+                           OpenStudio.get_absolute_path(pattern)
+                         else
+                           pattern
+                         end


Now, the pattern needs to be expanded only if it's an embedded one.

Eg if this is '*.gemspec', don't expand it to $(pwd)/*.gemspec which is going to be on your actual host filesystem always.

(Note: I suppose doing File.expand_path(pattern, base) was a possibility)

jmarrec · 2025-05-14T08:14:32Z

ruby/engine/embedded_help.rb

+      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)}"


loop on the embedded fileNames (which all start with ':/'), skip if they don't match the base

jmarrec · 2025-05-14T08:15:52Z

ruby/engine/embedded_help.rb

+        #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


Can fnmatch, afaik there's no need to override the flags.

jmarrec · 2025-05-14T08:28:39Z

Oh cool, I have new failures in bundle test on Ubuntu! Only when --loglevel Trace. Nice.

shorowit · 2025-05-14T11:56:20Z

Are we missing some kind of CI test for the Windows issue? Seems like we shouldn’t have to rely on someone installing OS on their machine to discover this problem.

jmarrec · 2025-05-14T13:12:11Z

Not sure what's up on CI 24.04, locally I have:

1/7 Test #4019: CLITest-test_bundle-bundle_native ............ Passed 23.26 sec
2/7 Test #4021: CLITest-test_bundle-bundle_no_install ........ Passed 25.96 sec
3/7 Test #4022: CLITest-test_bundle-no_bundle ................ Passed 27.83 sec
4/7 Test #4023: CLITest-test_bundle-bundle_default ........... Passed 27.96 sec
5/7 Test #4017: CLITest-test_bundle-bundle ................... Passed 35.26 sec
6/7 Test #4018: CLITest-test_bundle-bundle_git ............... Passed 37.80 sec
7/7 Test #4020: CLITest-test_bundle-bundle_native_embedded ... Passed 38.91 sec

(note: I no longer use rvm, but rbenv)

jmarrec · 2025-05-14T13:17:46Z

Are we missing some kind of CI test for the Windows issue? Seems like we shouldn’t have to rely on someone installing OS on their machine to discover this problem.

CI was (and somewhat still is, we have open issues tagged "Continuous Integration") so utterly bricked that this just didn't raise any alarms.

example: [CI] Windows and Ubuntu incremental hides failing unit tests #5328

I'm still going to add a quick test though: 6667e19

* rubygems.rb calls Gem::Specification.load_defaults * https://github.com/ruby/ruby/blob/f91480d7a671b1b114270a4b5e4d3c5aa6dabce9/lib/rubygems.rb#L1355 * That uses each_spec([Gem.default_specifications_dir]), * Gem.default_specifications_dir is now `:/ruby/gems/3.2.0/specifications/default"` * That ends up calling rubygems/util.rb#glob_files_in_dir * https://github.com/ruby/ruby/blob/f91480d7a671b1b114270a4b5e4d3c5aa6dabce9/lib/rubygems/util.rb#L100-L102 * Essentially calling `Dir.glob('*.gemspec', ':/ruby/gems/3.2.0/specifications/default')`

…lob works

ci-commercialbuildings · 2025-05-14T13:54:10Z

CI Results for 6667e19:

Ubuntu 24.04:
Ubuntu 22.04:
- OpenStudio-3.10.0-alpha+6667e19cbe-Ubuntu-22.04-x86_64.deb
- OpenStudio-3.10.0-alpha+6667e19cbe-Ubuntu-22.04-x86_64.tar.gz
Darwin:
- OpenStudio-3.10.0-alpha+6667e19cbe-Darwin-x86_64.dmg
- OpenStudio-3.10.0-alpha+6667e19cbe-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.10.0-alpha+6667e19cbe-Windows.exe
- OpenStudio-3.10.0-alpha+6667e19cbe-Windows.tar.gz
Code Coverage:

joseph-robertson · 2025-05-14T20:43:15Z

FWIW, using the Windows installer from this PR I no longer see "Segmentation fault" or "Failed to execute ..." for ruby_version or <script.rb> passed to the CLI.

kbenne · 2025-05-20T15:22:49Z

It doesn't seem any more fragile than what we have been doing all along. The fragility just seems to be highlighted when Ruby changes things. My main thought is that I'm glad you were able to make this work, but I'm still in favor of ditching the embedded system.

kbenne · 2025-05-20T15:13:14Z

ruby/bindings/InitRubyBindings.cpp


    openstudio::evalString(initScript);

+#ifndef _WIN32  // no io/console available


Wait, so why no irb on windows? It fails due to no io/console or because of the logging bug?

kbenne · 2025-05-20T15:14:27Z

ruby/engine/embedded_help.rb

      absolute_path = ':' + absolute_path
    else
-      absolute_path = File.expand_path p2
+      absolute_path = File.expand_path p


so we never called this function on a non embedded file?

jmarrec requested a review from kbenne May 14, 2025 07:59

jmarrec self-assigned this May 14, 2025

jmarrec added severity - Blocker component - CLI Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. component - Ruby bindings labels May 14, 2025

jmarrec commented May 14, 2025

View reviewed changes

jmarrec force-pushed the 5406-Ruby_CLI_Windows branch from 893f134 to e2ea133 Compare May 14, 2025 13:03

jmarrec added 8 commits May 14, 2025 15:17

Don't load IRB when LogLevel=Trace on windows

264de9d

OpenStudio.get_absolute_path is broken when not embedded

a32143d

Turn off debugging in glob

04dd5f1

Fixup fnmatch: was not finding IRB in Trace loglevel because of it

400c30a

Add some notes for future me/kyle

4bf26cb

put back override for FNM_EXTGLOB so CLITest-test_embedded_help-dir_g…

723ca8d

…lob works

#5216 - Add a basic openstudio --loglevel Trace ruby_version

6667e19

jmarrec force-pushed the 5406-Ruby_CLI_Windows branch from e2ea133 to 6667e19 Compare May 14, 2025 13:19

DavidGoldwasser added this to the OpenStudio SDK 3.10.0 milestone May 15, 2025

kbenne approved these changes May 20, 2025

View reviewed changes

DavidGoldwasser merged commit 697c657 into develop May 20, 2025
3 of 6 checks passed

DavidGoldwasser deleted the 5406-Ruby_CLI_Windows branch May 20, 2025 15:56


		openstudio::evalString(initScript);

		#ifndef _WIN32 // no io/console available

Fix #5406 - Regression in CLI on Windows: openstudio ruby_version fails on develop #5407

Fix #5406 - Regression in CLI on Windows: openstudio ruby_version fails on develop #5407

Uh oh!

Conversation

jmarrec commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Pull Request Author

Review Checklist

Uh oh!

jmarrec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec commented May 14, 2025

Uh oh!

shorowit commented May 14, 2025

Uh oh!

jmarrec commented May 14, 2025

Uh oh!

jmarrec commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ci-commercialbuildings commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseph-robertson commented May 14, 2025

Uh oh!

kbenne commented May 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jmarrec commented May 14, 2025 •

edited

Loading

jmarrec commented May 14, 2025 •

edited

Loading

ci-commercialbuildings commented May 14, 2025 •

edited

Loading