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 issues with release.yaml due to breaking changes in actions/upload-artifact and actions/download-artifact #173

Open
wants to merge 11 commits into
base: fastsim-2
Choose a base branch
from

Conversation

michael-okeefe
Copy link
Collaborator

@michael-okeefe michael-okeefe commented Dec 9, 2024

actions/upload-artifact and actions/download-artifact both were updated with breaking changes which caused release.yaml not to run anymore. (NOTE: there were deprecations so keeping with the old version was not an option). This attempts to fix the release mechanism to working order again.

Key change is that upload-artifact must now use unique names and dowload-artifact must be changed to merge all files into a single directory for twine to pick up.

  • ensure all wheels will build by running release.yml via actions
  • remove flag to prevent release mechanism to pypi
  • tag the release (suggest: v2.1.3+mod01)
  • re-run via the actual release mechanism (after merge): https://github.com/NREL/fastsim/releases/new

References:

actions/upload-artifact and actions/download-artifact both were updated with breaking changes which caused release.yaml not to run anymore. This attempts to fix the release mechanism to working order again.

Key change is that upload-artifact must now use unique names and dowload-artifact must be changed to merge all files into a single directory for twine to pick up.
Apple silicon builds have a requirement for a recent pip.
@michael-okeefe
Copy link
Collaborator Author

@calbaker @kylecarow I've gotten the release.yaml working to the point of building the Apple universal builds and that is where it seems to be failing. I ran into the same problem when trying to get things set up for FASTSim 3.

Would you both be OK with me setting up separate Apple Silicon and Apple Intel builds? Or do you have any thoughts on how to get a universal build working on cibuildwheel for Apple?

See here if you'd like to look at the latest build errors:

https://github.com/NREL/fastsim/actions/runs/12246134820/job/34161446298

Michael O'Keefe added 5 commits December 11, 2024 14:37
Attempt to use separate intel and apple silicon builds
vs trying to use the universal build for wheels. This
is faciliated by using macos-13 (Intel) and macos-14
(Apple Silicon).
The variable 'MACOSX_DEPLOYMENT_TARGET=10.12' is needed to
updated minimum supported macOS for this wheel.
@michael-okeefe michael-okeefe marked this pull request as ready for review December 12, 2024 00:08
@michael-okeefe
Copy link
Collaborator Author

Note: release is running clean (this version has the pypi deploy disabled):
https://github.com/NREL/fastsim/actions/runs/12286916954

@michael-okeefe michael-okeefe marked this pull request as draft December 13, 2024 14:43
@michael-okeefe michael-okeefe marked this pull request as ready for review December 17, 2024 22:49
@calbaker
Copy link
Collaborator

@michael-okeefe, could you investigate the failing checks in this PR?

Michael O'Keefe added 2 commits January 2, 2025 08:38
The `wheels.yml` file did not receive the required
updates and changes needed for building with Mac OS
for Apple Silicon AND Apple Intel. These changes have
now been copied over such that the `wheels.yml` file is
essentially the same as the first part of the `release.yml`
file. We may be able to eliminate the `wheels.yml` workflow
in the future although it is helpful as a check to confirm
that the `release.yml` file (at least, the "build" section)
will run without issue.
@michael-okeefe
Copy link
Collaborator Author

@michael-okeefe, could you investigate the failing checks in this PR?

@calbaker It turned out that this branch was just out of date. We had removed the wheels.yml workflow in another commit because it was unnecessary and was not working and that commit had just not been merged into this PR yet. The tests need to run still but I'm anticipating they will all run without issue now.

@calbaker
Copy link
Collaborator

calbaker commented Jan 6, 2025

@michael-okeefe, could you investigate the failing checks in this PR?

@calbaker It turned out that this branch was just out of date. We had removed the wheels.yml workflow in another commit because it was unnecessary and was not working and that commit had just not been merged into this PR yet. The tests need to run still but I'm anticipating they will all run without issue now.

My understanding is that the tests pass and this can be merged, right @michael-okeefe ?

@michael-okeefe
Copy link
Collaborator Author

@michael-okeefe, could you investigate the failing checks in this PR?

@calbaker It turned out that this branch was just out of date. We had removed the wheels.yml workflow in another commit because it was unnecessary and was not working and that commit had just not been merged into this PR yet. The tests need to run still but I'm anticipating they will all run without issue now.

My understanding is that the tests pass and this can be merged, right @michael-okeefe ?

@calbaker Yes, that is correct. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants