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 #5162 - Capture exit from Ruby #5166

Merged
merged 12 commits into from
May 2, 2024
Merged

Fix #5162 - Capture exit from Ruby #5166

merged 12 commits into from
May 2, 2024

Conversation

jmarrec
Copy link
Collaborator

@jmarrec jmarrec commented Apr 23, 2024

Pull request overview

Note: this does fix the Minitest return value, even with the minitest/autorun using at_exit

https://github.com/minitest/minitest/blob/ea9caafc0754b1d6236a490d59e624b53209734a/lib/minitest.rb#L66-L89

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 jmarrec added 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 Apr 23, 2024
@jmarrec jmarrec self-assigned this Apr 23, 2024
@jmarrec jmarrec requested a review from kbenne April 23, 2024 16:24
Copy link
Contributor

@kbenne kbenne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmarrec OK great. We need this. The question is when to merge?

I think @wenyikuang should tag the RC based on the current head of the develop branch, but if the two of you think we can squeak this in for RC1, then I'm ok with that too. Ultimately your call @wnykuang

@kbenne
Copy link
Contributor

kbenne commented Apr 23, 2024

I think we might get some new failures from ctest as a result of this. (which is a feature not a bug of course)

@jmarrec
Copy link
Collaborator Author

jmarrec commented Apr 23, 2024

Hummmm I no longer have any minitest output.

test_minitest.rb

require 'minitest'
require 'minitest/autorun'

class MyTest < Minitest::Test
  def test_fail
    assert 0 == 1
  end
end

@jmarrec jmarrec marked this pull request as draft April 23, 2024 16:40
@wenyikuang
Copy link
Collaborator

Eventually I need build the binary from scratch so its not a big difference.

@jmarrec
Copy link
Collaborator Author

jmarrec commented Apr 23, 2024

I'd say wait, I have some issues with it. Maybe it'll go in RC2.

e52af4d kinda fixes it, but I don't like it one bit

Minitest failures still end up exiting with an exitcode = 0 obviously...

Comment on lines +78 to +84
// ruby_cleanup calls ruby_finalize
int ex = ruby_cleanup(0);
if (ex != 0) {
fmt::print("RubyEngine return code was {}\n", ex);
exit(ex);
}
//ruby_finalize();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbenne here is the most important change.

Originally I had fixed it by using an at_exit block in executeRubyScriptCommand. THe Kernel.exit! was sufficient to propagate it to C++. But the issue was that this would be returning zero : openstudio -e "at_exit { exit 12 }". So I thought "Wouldn't it be more logical to install that at_exit in the RubyEngine itself?" and that's when I realized ruby has a ruby_cleanup which returns the final exit code, so we should use that

at_exit {{
  exit_code = ($! and $!.kind_of? SystemExit) ? $!.status : 0
  STDOUT.flush
  STDERR.flush
  if exit_code != 0
    Kernel.exit!(exit_code)
  end
}}

https://github.com/ruby/ruby/blob/af471c0e0127eea0cafa6f308c0425bbfab0acf5/include/ruby/internal/interpreter.h#L160-L181

Comment on lines +479 to +488
if (Python_EXECUTABLE)
add_test(NAME OpenStudioCLI.minitest_fail
COMMAND ${Python_EXECUTABLE} "${CMAKE_CURRENT_SOURCE_DIR}/test/run_minitest_fail.py" $<TARGET_FILE:openstudio>
)
if (SYSTEM_RUBY_EXECUTABLE)
add_test(NAME RubyTest.minitest_fail
COMMAND ${Python_EXECUTABLE} "${CMAKE_CURRENT_SOURCE_DIR}/test/run_minitest_fail.py" "${SYSTEM_RUBY_EXECUTABLE}"
)
endif()
endif()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new tests

@jmarrec jmarrec marked this pull request as ready for review April 26, 2024 16:32
@jmarrec jmarrec added this to the OpenStudio SDK 3.8.0 milestone Apr 29, 2024
@kbenne kbenne merged commit 373c37e into develop May 2, 2024
1 of 6 checks passed
@kbenne kbenne deleted the 5162_exit branch May 2, 2024 20:52
// ruby_cleanup calls ruby_finalize
int ex = ruby_cleanup(0);
if (ex != 0) {
fmt::print("RubyEngine return code was {}\n", ex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmarrec Did you mean to leave this in or was it for debugging? Seems weird that this is now printed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component - CLI component - Ruby bindings Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"exit 1" in ruby code is silently dismissed
5 participants