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

Fixup the python bindings workflow, add python 3.13 for windows, and use os.add_dll_directory for windows #5288

Merged
merged 8 commits into from
Nov 13, 2024
Merged

Fixup the python bindings workflow, add python 3.13 for windows, and use os.add_dll_directory for windows #5288

merged 8 commits into from
Nov 13, 2024

Conversation

jmarrec
Copy link
Collaborator

@jmarrec jmarrec commented Nov 6, 2024 •
edited
Loading

Pull request overview

Noticed the Python bindings workflow as failing for a bunch of platforms: https://github.com/NREL/OpenStudio/actions/runs/11677150372

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 Developer Issue Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. component - Python bindings labels Nov 6, 2024
@jmarrec jmarrec requested a review from kbenne November 6, 2024 11:22
@jmarrec jmarrec self-assigned this Nov 6, 2024
jmarrec
jmarrec commented Nov 6, 2024
View reviewed changes
python/module/openstudio.py
Comment on lines +48 to +53
import os
if os.name == 'nt':
# When we're using system python to load the **installed** C:\openstudio-X.Y-Z\Python stuff (not PyPi package)
# This allows finding openstudiolib.dll and the msvc ones in the bin/ folder while we're in the Python/ folder
# Otherwise you'd have to manually copy these DLLs from bin/ to Python/
os.add_dll_directory(os.path.abspath(os.path.join(os.path.dirname(__file__), '..', 'bin')))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://docs.python.org/3/library/os.html#os.add_dll_directory

@jmarrec
Copy link
Collaborator Author

jmarrec commented Nov 7, 2024 •
edited
Loading

@wenyikuang I added --verbose for the upload to testpypi to find out why it wasn't working.

https://github.com/NREL/OpenStudio/actions/runs/11721773404/job/32656795337?pr=5288#step:6:126

400 User 'cicommercialbuilding' does not have two-factor
authentication enabled. Please enable two-factor authentication before
attempting to upload to PyPI. See
https://test.pypi.org/help/#two-factor-authentication for more
information

Please enable 2FA for both testpypi and pypi.org

Alternatively, we could probably use https://docs.pypi.org/trusted-publishers/

@jmarrec
Bump python bindings action: add windows py313, bump macos to 12 (wil…
d7f7e42 
…l be removed in december) and use ubuntu 22.04
@jmarrec
my forked setup-python doesn't work for windows py313 because it's no…
2b8be92 
…t in the image cache

also -Wno-enum-constexpr-conversion isn't understand on older apple clang
@jmarrec
Try with a ternary like operator
b31e02c 
I'm referencing the id of that step...

https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#example
@jmarrec
Use a different step id
0e3cc37
@jmarrec
use os.add_dll_directory for windows when using the **installed** Pyt…
f2ce25b 
…hon/ folder with system python

https://docs.python.org/3/library/os.html#os.add_dll_directory
@jmarrec
Py_SetPath removed from Python 3.13. We should upgrade to use PyConfi…
ad5f550 
…g, but too late in the release cycle
@jmarrec
Why is testpypi upload failing? Try with --verbose
76e2246
@jmarrec
Copy link
Collaborator Author

jmarrec commented Nov 13, 2024

So the good news is that enabled 2FA restored the ability to upload to (test)PyPi.

The less good news is that the testing step for Ubuntu failed, but that's good we caught it early. This is only because since E+ doesn't provide packages for Ubuntu 20.04 anymore, I am building with 22.04, which needs GLIBC >= 2.32.0, but I was still testing on Ubuntu 20.04 which only has GLIBC 2.31.0. Let's just drop support for 20.04 altogether.

The mac test step is set to run on macos-11, which was removed.

Pushing, hopefully, a last commit.

@ci-commercialbuildings
Copy link
Collaborator

ci-commercialbuildings commented Nov 13, 2024 •
edited
Loading

CI Results for 4958299:

@jmarrec jmarrec merged commit a021de9 into develop Nov 13, 2024
27 of 28 checks passed
@jmarrec jmarrec deleted the actions_pypi branch November 13, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbenne kbenne Awaiting requested review from kbenne

Assignees

@jmarrec jmarrec

Labels
component - Python bindings Developer Issue 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.

Broken python bindings workflow
2 participants
@jmarrec @ci-commercialbuildings