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.

Sign up for GitHub

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

Jump to bottom

#4496 - Add gems cbor and msgpack and allow using a package from an openstudio-gems PR #4502

Merged
merged 10 commits into from Nov 19, 2021
Merged

#4496 - Add gems cbor and msgpack and allow using a package from an openstudio-gems PR #4502

merged 10 commits into from Nov 19, 2021

Conversation

jmarrec
Copy link
Collaborator

@jmarrec jmarrec commented Nov 16, 2021

Pull request overview

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
Support using the openstuido_gems packages produced by CI on a PR and…
a8993af 
… use NREL/openstudio-gems#51
@jmarrec
I added a version.txt file on oppenstudio-gems tar.gz, on my PR-51, s…
c89f363 
…o read that to make sure you use the right openstudio3-gems tar.gz especially when switching branches on CI.
@jmarrec jmarrec self-assigned this Nov 16, 2021
jmarrec
jmarrec commented Nov 16, 2021
View reviewed changes
CMakeLists.txt
set(OPENSTUDIO_GEMS_BASEURL "http://openstudio-resources.s3.amazonaws.com/dependencies")

# To use the package produced by a PR to https://github.com/NREL/openstudio-gems
set(USE_OPENSTUDIO_GEMS_PR TRUE)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would be set to FALSE normally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice option to enable custom gem builds

CMakeLists.txt
set(USE_OPENSTUDIO_GEMS_PR TRUE)
if (USE_OPENSTUDIO_GEMS_PR)
set(OPENSTUDIO_GEMS_BASEURL "http://openstudio-sdk-dependencies.s3.amazonaws.com/openstudio-gems")
set(OPENSTUDIO_GEMS_PR_NUMBER "PR-51")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the PR-number in question, as that's how jenkins uploads them.

CMakeLists.txt
Comment on lines +812 to +828
# If the extracted dir has a version.txt, it must match, otherwise remove it
if(EXISTS "${OPENSTUDIO_GEMS_DIR}/version.txt")
file(READ ${OPENSTUDIO_GEMS_DIR}/version.txt EXTRACTED_TAR_GZ_NAME)
string(STRIP ${EXTRACTED_TAR_GZ_NAME} EXTRACTED_TAR_GZ_NAME)
if(NOT EXTRACTED_TAR_GZ_NAME STREQUAL OPENSTUDIO_GEMS_ZIP_FILENAME)
message("Extracted gem dir ${OPENSTUDIO_GEMS_DIR} has the wrong version, removing it")
file(REMOVE_RECURSE "${OPENSTUDIO_GEMS_DIR}")
endif()
else()
# Until the openstudio-gems package from my PR-51 goes onto develop, assume you want to remove it to avoid leaving it in place when you switch back to develop
if(EXISTS "${OPENSTUDIO_GEMS_DIR}" AND NOT USE_OPENSTUDIO_GEMS_PR)
file(REMOVE_RECURSE "${OPENSTUDIO_GEMS_DIR}")
endif()
endif()

# If you do not have the extracted dir, then extract the tar.gz
if(NOT EXISTS "${OPENSTUDIO_GEMS_DIR}")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See my comment over there NREL/openstudio-gems#51 (comment) for why this is needed

Write the tar.gz name into a text file at the root of the extracted dir, so we can get it from CMake to ensure we always use the right version when switching branches: checking that we have the .tar.gz locally and it has the right MD5sum doesn't suffice to tell that the extracted directory is the right now, as you may have multiple openstudio3-gems tar.gzs.

@jmarrec jmarrec added the Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. label Nov 16, 2021
@jmarrec jmarrec mentioned this pull request Nov 16, 2021
Closed
@tijcolem
Copy link
Collaborator

@jmarrec This will be nice to have these. Good to see it takes up such little space too.
Looks like CI unit tests are failing:

https://ci.commercialbuildings.dev/blue/organizations/jenkins/openstudio-incremental-osx/detail/PR-4502/7/pipeline/#step-63-log-45

@jmarrec
Copy link
Collaborator Author

jmarrec commented Nov 17, 2021

The incremental jenkins runners would need a clean build. I launched a nightly build (back when I had both cbor and messagepack): https://ci.commercialbuildings.dev/blue/organizations/jenkins/openstudio-develop-nightly/detail/openstudio-develop-nightly/1309/pipeline/224

As can be seen, both ubuntus and the mac one are passing the tests with both cbor and msgpack:

3461/3466 Test #3462: CLITest-test_embedded_ruby-cbor ....................................................................................................... Passed 8.50 sec

3462/3466 Test #3463: CLITest-test_embedded_ruby-msgpack .................................................................................................... Passed 8.07 sec

I'll kill it because I KNOW windows will fail. Will relaunch with only msgpack.

@jmarrec
Copy link
Collaborator Author

jmarrec commented Nov 17, 2021

Relaunched full build with msgpack only at https://ci.commercialbuildings.dev/blue/organizations/jenkins/openstudio-develop-nightly/detail/openstudio-develop-nightly/1310/pipeline

@jmarrec
Copy link
Collaborator Author

jmarrec commented Nov 17, 2021

@tijcolem the full build is happy with it:

windows: https://ci.commercialbuildings.dev/blue/organizations/jenkins/openstudio-develop-nightly/detail/openstudio-develop-nightly/1310/pipeline#step-282-log-1192
3463/3467 Test #3463: CLITest-test_embedded_ruby-msgpack .................................................................................................... Passed 27.50 sec

@ci-commercialbuildings
Copy link
Collaborator

ci-commercialbuildings commented Nov 17, 2021 •
edited

CI Results for 56e4c0e:

@tijcolem tijcolem merged commit 2247c87 into develop Nov 19, 2021
@tijcolem tijcolem deleted the 4496_gem_cbor_msgpack branch November 19, 2021 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tijcolem tijcolem tijcolem approved these changes

Assignees

@jmarrec jmarrec

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add cbor or msgpack gem to CLI
3 participants
@jmarrec @tijcolem @ci-commercialbuildings