-
Notifications
You must be signed in to change notification settings - Fork 400
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
Post 24.1 Cleanups #10458
Post 24.1 Cleanups #10458
Conversation
@Myoldmopar it has been 7 days since this pull request was last updated. |
3 similar comments
@Myoldmopar it has been 7 days since this pull request was last updated. |
@Myoldmopar it has been 7 days since this pull request was last updated. |
@Myoldmopar it has been 7 days since this pull request was last updated. |
Alright, it's close. It is getting the packages built, and then starting the follow-up testing. But unfortunately the main test runner couldn't find the rest of the test package. I have added to the PYTHONPATH prior to running the test script, which is what you would do locally. If it doesn't work, I'm probably going to just gut the package down into a single Python file and eliminate this problem entirely. Another release test incoming... |
OH, it worked! OK, I'm going to undo some of the temporary changes here and then get this ready to merge. |
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.
Walkthrough done, if CI maintains good results, we should be good to merge this in.
path: build/EnergyPlus-*-Windows-${{ matrix.package-arch }}.zip | ||
if-no-files-found: error | ||
retention-days: 7 | ||
overwrite: false |
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.
@jmarrec for the main build job, I'm just adding the step of uploading the zip as an artifact. I am leaving the rest of it untouched, so it will continue to upload the package to the release as well. If there's a problem with it, so be it, but I thought it was better to let all the installers attempt to upload as normal.
@@ -21,7 +21,7 @@ jobs: | |||
fail-fast: false | |||
matrix: | |||
python-version: ["3.9"] | |||
os: [ubuntu-22.04, macos-13, windows-2019] | |||
os: [ubuntu-22.04] # , macos-13, windows-2019] |
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.
For now wheels are only going to work on Linux, and they aren't proper manylinux
wheels yet anyway. But it's close. This will be my next major effort.
repository-url: https://test.pypi.org/legacy/ | ||
user: __token__ | ||
password: ${{ secrets.TESTPYPIPW }} | ||
verbose: true |
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.
Added the command to actually deploy the wheel to PyPi, this is such a great step!
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.
In a subsequent effort, I'll modify this script to be able to deploy to either PyPi or TestPyPi on demand, as well as for each tag.
if-no-files-found: error | ||
retention-days: 7 | ||
overwrite: false | ||
|
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.
On all three platforms, I now upload the tarball/zip as an artifact in the first job, and then test the artifact in a fresh container in a second job.
@@ -219,7 +219,7 @@ if(BUILD_TESTING) | |||
"${CMAKE_SYSTEM_NAME}-${CMAKE_CXX_COMPILER_ID}${ARCH_FLAG}" | |||
CACHE STRING "Identifier for this device configuration") | |||
endif() | |||
|
|||
set_property(GLOBAL PROPERTY CTEST_TARGETS_ADDED 1) # This avoids all the CTest Nightly, Continuous, etc. tests. |
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.
Simply hush up a bunch of random ctest target projects, no functional change.
check_call([make_tool, '-j', str(cpu_count() - 2), 'energyplus'], cwd=str(build_dir)) | ||
else: | ||
products_dir = '/tmp/EnergyPlus-24.1.0-241fc81186-Linux-Ubuntu22.04-x86_64' | ||
|
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.
Just random things added to the debug script. I doubt anyone else will be using this, so just ignore.
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.
All the remaining changes below are the contents of the ep_testing library that we are using to test EnergyPlus packages!
Alright, I am pumped about adding this automatic package testing. Soon I will make a pass over the testing code to clean any oddities up, and also add more testing. If we ever encounter packaging issues again, we should be able to add tests in to catch them. This is a big deal. I'm merging this. |
Pull request overview
The release of 24.1 is behind us, and seems to be happy. However, the release was made difficult by GitHub making a container image update 3 days before our release, which led to failing packages and a stressful last minute investigation. The root of the problem is that GHA updated the Windows image to bump CMake 3.28.3 to 3.29.0. And there was a behavior change which led to our packages missing the Python DLL.
While I was doing regular package testing, it would be much better if the package testing scripts happened here, right in this same repo. So I've brought them in. I'll do some other minor changes here that I find along the way.
It's happy on all three platforms now.
FYI @jmarrec