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

ci: use reusable workflow for MacOs build #9392

Draft
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

fedordikarev
Copy link
Contributor

@fedordikarev fedordikarev commented Oct 14, 2024

Do not merge, for dev and testing purposes

Problem

Summary of changes

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@fedordikarev fedordikarev added the run-extra-build-macos When placed on a PR, tells the CI to run a build on macOS. No unit tests are run, though. label Oct 14, 2024
Comment on lines 28 to 39
- name: Check for Postgres changes
uses: dorny/paths-filter@1441771bbfdd59dcd748680ee64ebd8faab1a242 #v3
id: postgres_changes
with:
token: ${{ github.token }}
filters: |
postgres-v14: ['vendor/postgres-v14/**', 'Makefile', '.github/workflows/build-macos.yml', 'pgxn/**']
postgres-v15: ['vendor/postgres-v15/**', 'Makefile', '.github/workflows/build-macos.yml', 'pgxn/**']
postgres-v16: ['vendor/postgres-v16/**', 'Makefile', '.github/workflows/build-macos.yml', 'pgxn/**']
postgres-v17: ['vendor/postgres-v17/**', 'Makefile', '.github/workflows/build-macos.yml', 'pgxn/**']
base: ${{ github.event_name != 'pull_request' && (github.event.merge_group.base_ref || github.ref_name) || '' }}
ref: ${{ github.event_name != 'pull_request' && (github.event.merge_group.head_ref || github.ref) || ''}}
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about having this part in the main build_and_test workflow and trigger build-macos only if there're related changes.

We might want to consider using https://github.com/tj-actions/changed-files (instead of dorny/paths-filter), I found its API nicer

postgres_changes: ${{ steps.postgres_changes.outputs.changes }}
steps:
- name: Checkout
uses: actions/checkout@6ccd57f4c5d15bdc2fef309bd9fb6cc9db2ef1c6 # v4.1.7
Copy link
Member

Choose a reason for hiding this comment

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

We're ok to use the latest tag (like @v4) for "official" actions from actions org

contains(github.event.pull_request.labels.*.name, 'run-extra-build-*') ||
github.ref_name == 'main'
timeout-minutes: 30
runs-on: macos-14
Copy link
Member

Choose a reason for hiding this comment

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

I've find out, that macos-15 is available, let's switch it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems something has changed in Github runners, as build started to fail on both macos-14 and macos-15 due to ICU library not found.
and there are other complain on that library missing from the image: actions/runner-images#10989

I will anyway get back to macos-15, just to keep track of why build fails now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-extra-build-macos When placed on a PR, tells the CI to run a build on macOS. No unit tests are run, though.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants