-
Notifications
You must be signed in to change notification settings - Fork 137
Create a release file containing submodules #825
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #825 +/- ##
==========================================
- Coverage 91.84% 89.34% -2.50%
==========================================
Files 37 61 +24
Lines 4976 8566 +3590
Branches 0 939 +939
==========================================
+ Hits 4570 7653 +3083
- Misses 406 913 +507 ☔ View full report in Codecov by Sentry. |
.github/workflows/release_file.yml
Outdated
- name: Add file to release | ||
run: | | ||
cd ${{ env.release_name }} || die | ||
gh release upload ${{ github.ref_name }} ../${{ env.release_name }}.tar.gz |
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.
Does this override whatever Github's default release packaging is? Looks great, though I don't have much ability to review or test this. Should we just see what happens on the next release?
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.
It adds an additional file to the list of assets e.g. manifold-2.4.5.tar.gz
Glad you didn't review it - there were a couple of bits that were not so great (updated now), including relating to my initial comment - now it will strip the leading v from the tag only if it exists.
I also added a step to log the sha256sum of the created file.
I have tested it, publishing and creating a new tag in GitHub. So it should be good to go.
Don't know if you have seen but there is some interesting GitHub stuff in development for automatically registering signatures for artifacts without having to manage keys:
https://github.blog/2024-05-02-introducing-artifact-attestations-now-in-public-beta/
But it is beta at the moment.
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.
Sounds good, I'll trust you. Looks like the only problem is our CI - it started misbehaving yesterday for no apparent reason. I'm hoping M$ fixes it soon, though if you have any ideas what could be randomly canceling our jobs or how to work around it, I'd love to know! I already tried a few things to no avail: #826.
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.
Looks like the CI started working again - can you prod this PR with a minor commit to trigger the CI? My manual triggering doesn't rerun the randomly cancelled jobs.
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.
No problem - gave me the opportunity to remove a stray extra space after tar
.
I rebased it because the tests have changed. But thinking about it I suppose that wasn't really the point as this PR doesn't change anything. Sorry, at least it is a short one.
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.
No problem - in general though I prefer no rebasing and no force-pushing. We do squash and merge anyway, so there's no history pollution. Merging makes things much easier to review because it keeps commits separate. Plus it's safer.
Fixes #783
This does assume the release tag starts with v. If this workflow checked and then exited without creating a file that could be a problem.