-
Notifications
You must be signed in to change notification settings - Fork 185
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
Move the Release workflow to GitHub Actions #4518
Conversation
This pull request has been linked to Shortcut Story #36530: Release with github actions.. |
They also specify the minimum macOS version.
Hopefully fixes the OOMs.
The workflow has succeeded on my fork, correctly publishing the release artifacts. This is ready for review. |
And flow the option through the superbuild.
bece187
to
46f2341
Compare
Release artifacts are now produced for every PR. They can be viewed from the workflow's summary page. I took a look at one of them and its files seemed well-placed; most importantly this line in Tomorrow I will try running the tests for the Java API on Windows with these binaries. |
.github/workflows/release.yml
Outdated
env: | ||
# We target macOS 11 at minimum according to CRAN policy. | ||
# See https://stat.ethz.ch/pipermail/r-package-devel/2023q4/010078.html | ||
MACOSX_DEPLOYMENT_TARGET: 11 |
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.
We need to use the same target in regular CI 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.
Moved to the CMake build options and removed setting the MACOSX_DEPLOYMENT_TARGET
environment variables.
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.
Reverted after moving back to targeting 10.14. Regular CI cannot run while targeting 10.14; some of the code will not be able to compile.
.github/workflows/release.yml
Outdated
cmake_args: -DCMAKE_OSX_ARCHITECTURES=arm64 | ||
triplet: arm64-osx-release | ||
runs-on: ${{ matrix.os }} | ||
container: ${{ matrix.manylinux && 'quay.io/pypa/manylinux2014_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.
I would lean toward pinning the image version in order to avoid random breakage when it is changed (which has happened in the past).
Dependabot might (?) be able to update the image on a schedule, which would let us ensure smooth changes (not necessary in this PR though).
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.
Pinned. I think the Docker section of the Dependabot docs refers to images in Dockerfiles. I don't think these containers can be automatically updated; they are random strings in the workflow file.
And remove an unused option.
…re building a static library. Matches the previous behavior where the targets for the dependencies were defined only in that case.
By moving back to 10.14, some of our test code will not be able to build.
CI should be green after merging #4532. |
@KiterLuc the release workflow is green, the last conflicts were resolved without changes, and the CI failure on Azure seems like network issue. Can we merge this? |
- name: Archive installed artifacts (non-Windows) | ||
if: ${{ !startsWith(matrix.platform, 'windows') }} | ||
run: | | ||
tar -czf ${{ steps.get-values.outputs.archive_name }}.tar.gz -C dist . |
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.
As a follow-up (soon) we should turn this into a target -- it is very useful to be able to run make release
and get usable packages. Also reduce coupling to the CI language.
Fixes regression introduced in #4518. --- TYPE: BUG DESC: Fix regression when libraries were sometimes installed in the `lib64` directory.
- name: Set variables | ||
id: get-values | ||
run: | | ||
release_hash=$( echo ${{ github.sha }} | cut -c-8 - ) |
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.
Breadcrumb for later: this switched from 7-nybble hashes in filenames to 8-nybble hashes which broke some dependent packages. See #4599 for a fix.
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.
[SC-38683](https://app.shortcut.com/tiledb-inc/story/38683) Fixes a regression introduced in #4518. --- TYPE: BUILD DESC: Fix regression where release artifacts had 8-digit commit hashes.
[SC-38683](https://app.shortcut.com/tiledb-inc/story/38683) Fixes a regression introduced in #4518. --- TYPE: BUILD DESC: Fix regression where release artifacts had 8-digit commit hashes. (cherry picked from commit 31500c5)
SC-36530
This PR updates the Release workflow to run on top of GitHub Actions. The release builds now also run on every PR.
To test the new workflow I am making dummy releases to my fork. This PR will be ready for review once that succeeds.
PR also increases the minimum targeted macOS version for release artifacts to 11TYPE: NO_HISTORY
DESC: Move the Release workflow to GitHub Actions.