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

Set include and link dirs explicitly for macOS #162

Merged
merged 1 commit into from
Apr 30, 2024
Merged

Conversation

oschwald
Copy link
Member

No description provided.

env:
CFLAGS: "-Werror -Wall -Wextra"

- name: Test with tox (system libmaxminddb)
run: tox
if: matrix.platform != 'macos-latest'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if its necessarily a better strategy, but one pattern I've used for platform specific github action workarounds is to set environment variables for specific platforms. Something like:

- name: "Work around macos arm64 homebrew directory changes"
  if: runner.os == 'macos' and runner.arch = 'arm64'
  run: |
    echo "CFLAGS=-I/opt/homebrew/include" >> $env:GITHUB_ENV
    echo "LDFLAGS=-L/opt/homebrew/lib" >> $env:GITHUB_ENV

- name: Build with Werror and Wall
    run: python setup.py build
    env:
      CFLAGS: "${{ env.CFLAGS }} -Werror -Wall -Wextra"

- name: Test with tox (system libmaxminddb)
    run: tox

Copy link
Member Author

Choose a reason for hiding this comment

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

That does seem a bit cleaner. I can try doing that.

@@ -44,36 +44,19 @@ jobs:
run: sudo apt install libmaxminddb-dev
if: matrix.platform == 'ubuntu-latest'

- name: "Work around macos arm64 homebrew directory changes"
if: runner.os == 'macos' and runner.arch = 'arm64'
Copy link
Contributor

@ugexe ugexe Apr 30, 2024

Choose a reason for hiding this comment

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

This might have to be macOS and ARM64 actually

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like it worked. 🤷

Copy link
Contributor

@ugexe ugexe Apr 30, 2024

Choose a reason for hiding this comment

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

Yeah I'm confused. I guess the workflow file being invalid doesn't show as a failure? It should be

if: runner.os == 'macOS' && runner.arch == 'ARM64'

See Annotations from https://github.com/maxmind/MaxMind-DB-Reader-python/actions/runs/8897863643

[Invalid workflow file: .github/workflows/test-libmaxminddb.yml#L48](https://github.com/maxmind/MaxMind-DB-Reader-python/actions/runs/8897863643/workflow)
The workflow is not valid. .github/workflows/test-libmaxminddb.yml (Line: 48, Col: 13): Unexpected symbol: 'and'. Located at position 22 within expression: runner.os == 'macos' and runner.arch = 'arm64'

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I would have expected some indication that failed in the build statuses or via email. I'll fix that up.

@@ -44,36 +44,19 @@ jobs:
run: sudo apt install libmaxminddb-dev
if: matrix.platform == 'ubuntu-latest'

- name: "Work around macos arm64 homebrew directory changes"
if: runner.os == 'macos' and runner.arch = 'arm64'
Copy link
Contributor

Choose a reason for hiding this comment

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

Also runner.arch = -> runner.arch ==

@oschwald oschwald force-pushed the greg/fix-macos-build branch 3 times, most recently from 1d0a322 to 6cd5555 Compare April 30, 2024 16:24
@ugexe
Copy link
Contributor

ugexe commented Apr 30, 2024

Hmmm. I found actions/runner-images#9266 (reply in thread) which mentions:

The problem is that while this maps on X64 for some reason the runner images use the lowercase arm64 rather than following the convention with ARM64. Perhaps this is the issue that needs fixing instead?

So maybe it does need to be arm64, unlike what is mentioned on https://docs.github.com/en/actions/learn-github-actions/variables 😞

@oschwald
Copy link
Member Author

oschwald commented Apr 30, 2024

So maybe it does need to be arm64

I am not sure that is the issue. That seems to be running. I thought it might be $env:GITHUB_ENV vs "$GITHUB_ENV", which is mentioned here, but that didn't seem to help.

Edit: actually it was that. I just introduced an unrelated change accidentally.

echo "CFLAGS=-I/opt/homebrew/include" >> "$GITHUB_ENV"
echo "LDFLAGS=-L/opt/homebrew/lib" >> "$GITHUB_ENV"

- name: Build with Werror and Wall (not macOS)
Copy link
Contributor

Choose a reason for hiding this comment

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

The (not macOS) can be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I force-pushed it as a single commit to reduce the commit noise.

@ugexe ugexe merged commit b593356 into main Apr 30, 2024
63 checks passed
@ugexe ugexe deleted the greg/fix-macos-build branch April 30, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants