Skip to content

Conversation

@SiarheiFedartsou
Copy link
Member

Needed to be able to build bindings for arm64 based Linux... I tested it in my fork SiarheiFedartsou#1, seems to work

- "valhalla_python"
paths-ignore:
- '*.md'
pull_request:
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably a good idea to have CI running for PRs...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah totally! Didn't ever expect PRs tbh😄 and I usually commit directly to master in this repo

Copy link
Member

Choose a reason for hiding this comment

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

Can you also ignore md files?

Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Oct 20, 2025

Choose a reason for hiding this comment

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

Do you want me to create yet another PR? :) Now I understand that it was merged by mistake 😄

- name: Get branch name
run: |
echo "BRANCH=${GITHUB_REF##*/}" >> $GITHUB_ENV
echo "REPO_LOWERCASE=$(echo ${{ github.repository }} | tr '[:upper:]' '[:lower:]')" >> $GITHUB_ENV
Copy link
Member Author

Choose a reason for hiding this comment

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

Makes it properly working on forks: lowercase is needed because Docker doesn't allow any uppercase in repo name (SiarheiFedartsou/manylinux must be siarheifedartsou/manylinux)

Copy link
Member

Choose a reason for hiding this comment

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

Ah I thought that was only the "docker" docker registry.

docker push ghcr.io/${{ env.REPO_LOWERCASE }}:2_28_${{ matrix.platform }}_${{ env.BRANCH }}
docker image rm quay.io/pypa/manylinux_2_28_${{ matrix.platform }}:${{ github.sha }}
create_manifest:
Copy link
Member Author

Choose a reason for hiding this comment

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

Creates "magic" image which contains both architectures under the same name.

@SiarheiFedartsou
Copy link
Member Author

Btw @nilsnolde this is ready for review 😄

create_manifest:
name: Create multi-arch manifest
runs-on: ubuntu-24.04
Copy link
Member

Choose a reason for hiding this comment

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

Could do "-latest" where we can

pushd "${FREEXL_ROOT}"
# Update config.guess and config.sub for aarch64 support
if [ -f /usr/share/automake-*/config.guess ]; then
cp -f /usr/share/automake-*/config.guess .
Copy link
Member

Choose a reason for hiding this comment

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

This I don't really understand. We're copying some config stuff from automake?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, tbh it is something what I got as a tip from Claude Code when doing this(yes, these days it is how people actually do software engineering 😂 ). My understanding of what happens: these dependencies have these config.guess and config.sub in the root, but of quite old version which have no idea about aarch64, so we simply copy them from our own automake (which is modern enough to know about aarch64) to make it working. See this SO thread: https://stackoverflow.com/questions/4810996/how-to-resolve-configure-guessing-build-type-failure

- "valhalla_python"
paths-ignore:
- '*.md'
pull_request:
Copy link
Member

Choose a reason for hiding this comment

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

Can you also ignore md files?

@nilsnolde nilsnolde merged commit 2d772fd into valhalla:valhalla_python Oct 20, 2025
3 checks passed
@nilsnolde
Copy link
Member

Oops actually just wanted to approve😅

Thanks for doing this.

@SiarheiFedartsou
Copy link
Member Author

Oops actually just wanted to approve😅

Thanks for doing this.

I will make a follow up PR 😂

@SiarheiFedartsou
Copy link
Member Author

#2

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