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

OpenStudio CLI #324

Merged
merged 46 commits into from
Aug 30, 2018
Merged

OpenStudio CLI #324

merged 46 commits into from
Aug 30, 2018

Conversation

anyaelena
Copy link
Member

Closes #269

@anyaelena anyaelena changed the title 270 241 develop 270 241 develop - DO NOT MERGE Apr 18, 2018
@anyaelena
Copy link
Member Author

anyaelena commented Apr 18, 2018

Tests are passing but I've hardcoded the full path to the openstudio executable for both Windows and Linux/OSX. This seems like a bad idea, but does this code only run on workers with scripted configuration?

If we do not want to hardcode path to executable (and we probably don't), we should be able to either:

  1. don't specify path but ensure included in PATH environment variable: This won't currently work on Linux/OSX, but it looks like may for newer versions of OpenStudio (this). Note that in Appveyor set up, it does not look like C:\projects\openstudio\bin is in PATH
  2. Add a OPENSTUDIO_LOCATION ENV variable for all environments.

This begs the question: If we need to change ENV variables in the CI setup for our tests to pass, where else do we need to change them so that PAT doesn't break? What else should I consider?
@nllong @rHorsey

@anyaelena
Copy link
Member Author

Also, per @nllong's comment on the related issue, i've set this to timeout after 2 hours.

@rHorsey
Copy link
Contributor

rHorsey commented Apr 18, 2018

@anyaelena Will the hardcoded path issue be fixed with the closure of NREL/OpenStudio#2911 ? If so, I'm happy to wait for OS 2.5.1 to be released & then rerun the tests & merge then if you are. Maybe also @macumber

@rHorsey
Copy link
Contributor

rHorsey commented Apr 18, 2018

Sorry - I can't help myself. I really love labels in github. I don't really know why.

@anyaelena
Copy link
Member Author

anyaelena commented Apr 18, 2018

@rHorsey I had to hardcode path to executable in for Windows as well, which is not a known OS bug.
I assume this is because the executable directory C:\projects\openstudio\bin was not in PATH. and then i also assume that is was not in PATH because calling openstudio via workflow gem leaned on this which in tern uses the RUBYLIB var (i actually don't see where this is used in the code, but it's clearly needed to point to the os ruby wrapper).

So:

  1. OS update will probably remove hardcoded path requirement for Linux/OSX, since I observed exactly the issue that they have addressed.
  2. updating PATH in Appveyor setup might allow tests to pass without hardcoded path to openstudio.exe. I will test this.
  3. ?? I need someone to talk to me about how PAT/OSS work together so i can understand whether this is well tested with our CI and feel comfortable that requiring a change to PATH won't break this.

@anyaelena anyaelena changed the title 270 241 develop - DO NOT MERGE 270 241 develop Apr 18, 2018
@anyaelena anyaelena changed the base branch from 241-develop to develop April 27, 2018 02:19
@anyaelena
Copy link
Member Author

@shorowit would like to test this branch

@anyaelena
Copy link
Member Author

@nllong @rHorsey does this error mean anything to you? we are getting it for unit tests in osx and linux

@anyaelena anyaelena requested a review from rHorsey August 7, 2018 20:43
@rHorsey
Copy link
Contributor

rHorsey commented Aug 8, 2018

I'm building an AMI (2.6.1-rc2) off of this tonight so David can beat it up tomorrow with lots of gem switching.

@rHorsey
Copy link
Contributor

rHorsey commented Aug 29, 2018

AMI 2.6.1-cli0 has been built against 43291c4 for testing.

@rHorsey
Copy link
Contributor

rHorsey commented Aug 29, 2018

Handed off to @DavidGoldwasser to test 2.6.1-cli0

@anyaelena anyaelena merged commit 3fbb090 into develop Aug 30, 2018
@anyaelena anyaelena deleted the 270-241-develop branch August 30, 2018 19:59
@DavidGoldwasser
Copy link

I have tested analysis finalization scripts on build of develop branch of PAT and a custom server setup. I was able to read from and write to the database.

@anyaelena anyaelena restored the 270-241-develop branch September 12, 2018 15:32
@anyaelena anyaelena deleted the 270-241-develop branch September 26, 2018 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants