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

update publish workflow and add intel macos build #991

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

daeho-ro
Copy link
Contributor

As a request of issue comment, I opened a PR for adding intel macos workflow and summarize it as matrix.

@akshayitzme
Copy link
Collaborator

@daeho-ro , Thank you for raising this PR , changing to the matrix strategy simplifies the workflow 👍🏻 .

During a dry run, I identified some variable misconfigurations. The current approach creates jobs for every possible combination of variables, which can be inefficient.

strategy:
matrix:
os: [macos-latest, macos-latest-large, ubuntu-latest, windows-2019]
opt: [--mac, --mac, --linux, --win]
dist: [dist-macOS-arm, dist-macOS-x86_64, dist-linux, dist-windows]

For example:
This will run fine, {os: "macos-latest", opt: "--mac", dist: "dist-macOS-arm"}
but this doesnt, {os: "macos-latest", opt: "--win", dist: "dist-windows"}

To address this, I propose using the include list within the matrix strategy.

jobs:
  build:
    runs-on: ${{ matrix.os }}

    strategy:
      matrix:
        include:
          - os: macos-14
            opt: '--mac'
            dist: 'dist-macOS-arm'

          - os: macos-13
            opt: '--mac'
            dist: 'dist-macOS-x86_64'

          - os: ubuntu-latest
            opt: '--linux'
            dist: 'dist-linux'

          - os: windows-2019
            opt: '--win'
            dist: 'dist-windows'

    steps:

On the dry run, macos-latest-large didn't work due to some reasons. I tried out different runners and this worked better.

OS Runner
Mac x86, x64 macos-13
Mac Arm macos-14

Ref: Publish 107

Could you make the mentioned changes ?.

@daeho-ro
Copy link
Contributor Author

Sure, I have updated!
Matrix element should be a list of param map not lists.

@akshayitzme akshayitzme merged commit 24ab053 into frappe:master Oct 23, 2024
4 checks passed
@daeho-ro daeho-ro deleted the update-publish-workflow branch October 23, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants