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

automatically build library with cmake and test with ctest #390

Merged
merged 1 commit into from
Feb 1, 2025

Conversation

zacharyburnett
Copy link
Contributor

@zacharyburnett zacharyburnett commented Nov 7, 2024

run cmake and make install in GitHub Actions to test the build

@jmr
Copy link
Member

jmr commented Nov 7, 2024

Thanks for trying this. I authorized and triggered the workflows. They all fail.

https://github.com/google/s2geometry/actions/runs/11724915532/job/32662569678?pr=390

Getting action download info
Error: Unable to resolve action `actions/checkout@v5`, unable to find version `v5`

Doesn't this also need to do something to install the dependencies like abseil-cpp?

@zacharyburnett
Copy link
Contributor Author

Thanks for trying this. I authorized and triggered the workflows. They all fail.

https://github.com/google/s2geometry/actions/runs/11724915532/job/32662569678?pr=390

Getting action download info
Error: Unable to resolve action `actions/checkout@v5`, unable to find version `v5`

oof, I always forget it only has v4 😅 I'll just pin it to a specific release

Doesn't this also need to do something to install the dependencies like abseil-cpp?

oh yes, I forgot about abseil

@jmr
Copy link
Member

jmr commented Nov 7, 2024

oof, I always forget it only has v4 😅 I'll just pin it to a specific release

Can you use v4 instead of v5? Or not specify a version? Why is a hash better?

I triggered it again for another batch of failures.

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Nov 7, 2024

oof, I always forget it only has v4 😅 I'll just pin it to a specific release

Can you use v4 instead of v5? Or not specify a version? Why is a hash better?

it's better to use a commit hash for security, because if someone takes control of the upstream action repo they could theoretically retag an existing tag to point to malicious code

It's not a pressing issue though, so we could just change it to v4 here, you're right

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Nov 7, 2024

thank you for being so patient as I iterate on this workflow! I'm attempting to make it easier to obtain wheels for s2geometry such that we could maybe use this downstream as a backend for our spherical_geometry package

@jmr
Copy link
Member

jmr commented Nov 7, 2024

I ran it again.

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Nov 7, 2024

I'm able to run actions on my fork: https://github.com/zacharyburnett/s2geometry/actions

I'll try to get this running

@jmr
Copy link
Member

jmr commented Nov 8, 2024

Install the project...
-- Install configuration: ""
CMake Error at absl/base/cmake_install.cmake:46 (file):
  file cannot create directory: /usr/local/lib/pkgconfig.  Maybe need
  administrative privileges.
Call Stack (most recent call first):
  absl/cmake_install.cmake:47 (include)
  cmake_install.cmake:47 (include)

You probably need -DCMAKE_INSTALL_PREFIX=... for abseil-cpp.

I use -DCMAKE_PREFIX_PATH=/tmp/absl-install-17 -DCMAKE_INSTALL_PREFIX=/tmp/absl-install-17 -DABSL_ENABLE_INSTALL=ON -DBUILD_TESTING=off -DCMAKE_VERBOSE_MAKEFILE=on -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_CXX_STANDARD=17, but I'm not sure how many of these are really needed.

Then build s2geometry with -DCMAKE_PREFIX_PATH=/tmp/absl-install-17.

@zacharyburnett
Copy link
Contributor Author

Install the project...
-- Install configuration: ""
CMake Error at absl/base/cmake_install.cmake:46 (file):
  file cannot create directory: /usr/local/lib/pkgconfig.  Maybe need
  administrative privileges.
Call Stack (most recent call first):
  absl/cmake_install.cmake:47 (include)
  cmake_install.cmake:47 (include)

You probably need -DCMAKE_INSTALL_PREFIX=... for abseil-cpp.

I use -DCMAKE_PREFIX_PATH=/tmp/absl-install-17 -DCMAKE_INSTALL_PREFIX=/tmp/absl-install-17 -DABSL_ENABLE_INSTALL=ON -DBUILD_TESTING=off -DCMAKE_VERBOSE_MAKEFILE=on -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_CXX_STANDARD=17, but I'm not sure how many of these are really needed.

Then build s2geometry with -DCMAKE_PREFIX_PATH=/tmp/absl-install-17.

ah, thank you! I apologize, I'm not familiar with building C++ at all, so I appreciate your help

@zacharyburnett zacharyburnett changed the title build Python wheels in CI build library and also Python wheels in CI Nov 8, 2024
@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Nov 8, 2024

from testing on the fork, the cmake job seems to succeed building now, but the cibuildwheel still fails:
image

@jmr
Copy link
Member

jmr commented Nov 8, 2024

How do you make cibuildwheel show the cmake output? I can't see why it failed.

https://github.com/google/s2geometry/actions/runs/11747466338/job/32729571493?pr=390

@jmr
Copy link
Member

jmr commented Nov 8, 2024

https://github.com/google/s2geometry/actions/runs/11747466338/job/32729571493?pr=390#step:4:505

The s2geometry cmake needs to be pointed at where abseil-cpp was installed.

something happened to CMAKE_PREFIX_PATH

https://github.com/google/s2geometry/actions/runs/11747466338/job/32729571493?pr=390#step:4:473

@jmr
Copy link
Member

jmr commented Nov 8, 2024

nproc doesn't exist on macOS. sysctl -n hw.physicalcpu or logicalcpu is the equivalent.

@zacharyburnett zacharyburnett force-pushed the ci/build branch 8 times, most recently from ed5b5e7 to 9614d07 Compare November 11, 2024 16:24
@zacharyburnett zacharyburnett marked this pull request as ready for review November 11, 2024 16:25
@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Nov 11, 2024

@jmr Thank you for being so patient as I figured this out. As you can see in the test run at https://github.com/zacharyburnett/s2geometry/actions/runs/11782372194, I was able to get the wheels to build and upload as artifacts to the run. It turns out that the reason why cibuildwheel did not see the built Abseil library, even when installed to /usr/local, was because cibuildwheel sets up another container to isolate the build environment, and it wasn't available there. I removed usage of pypa/cibuildwheel in favor of just using the tool it uses (pypa/build) to build the wheels.

We could also set the workflow to upload wheels to PyPI if triggered by a release; however, currently the s2geometry project slot is taken by an older fork of this repo that doesn't seem to have been updated recently: https://pypi.org/project/s2geometry/. I'm sure if you reached out to @figroc they could transfer the PyPI project to your team, and then it would just be a matter of adding the following to the bottom of .github/workflows/build.yml:

    - if: (github.event_name == 'release') && (github.event.action == 'released')
      uses: pypa/gh-action-pypi-publish@15c56dba361d8335944d31a2ecd17d700fc7bcbc # v1.12.2

Then you can add build.yml as a trusted publisher on the PyPI side (https://docs.pypi.org/trusted-publishers/adding-a-publisher/) and it'll be all set up to automatically upload wheels to PyPI upon every release in this repo.

@zacharyburnett zacharyburnett force-pushed the ci/build branch 5 times, most recently from 09eae89 to edd019c Compare December 5, 2024 18:24
@zacharyburnett
Copy link
Contributor Author

so I've been QUITE dumb 😅

All this time I've been trying to recreate cibuildwheel's functionality because I didn't know how to include the built version of abseil-cpp in the container, BUT I neglected to realize that I could have just been installing it with yum and brew (see #390 (comment)). I'm testing with cibuilwheel now, with a before-all command to install the requisite libraries in the config

@zacharyburnett zacharyburnett force-pushed the ci/build branch 8 times, most recently from ac53057 to 6322abf Compare December 5, 2024 19:45
@zacharyburnett zacharyburnett marked this pull request as draft December 5, 2024 19:46
@zacharyburnett
Copy link
Contributor Author

taking this back into draft until I can figure out the magic config for cibuildwheel that'll work

@jmr
Copy link
Member

jmr commented Dec 5, 2024

Ok, thanks for working so hard on this.

@jmr
Copy link
Member

jmr commented Jan 25, 2025

I think getting some automated test running would still be a great improvement. Could you just take out/disable the failing macOS parts? That could be a separate PR when it's ready.

@zacharyburnett zacharyburnett changed the title build library and also Python wheels in CI build and test CMake project in CI Jan 26, 2025
@zacharyburnett zacharyburnett marked this pull request as ready for review January 26, 2025 20:30
@zacharyburnett
Copy link
Contributor Author

@jmr that should do it, I separated out the Python wheel building / testing into #406

@zacharyburnett zacharyburnett changed the title build and test CMake project in CI automatically build library with cmake and test with ctest Jan 29, 2025
runs-on: ${{ matrix.runs-on }}
timeout-minutes: 30
steps:
- uses: zacharyburnett/setup-abseil-cpp@de39f445295c887839e30c864ffbbb1c0231bc83 # 1.0.5
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering, what's the difference between setup-abseil-cpp and using vcpkg or homebrew to install abseil-cpp? I see both vcpkg and homebrew are installed on the runner images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a while ago, but I think I remember that the homebrew recipe was not configured such that it could link properly, which is why I resorted to building it manually; I did not try vcpkg though

@jmr jmr merged commit f91ed0f into google:master Feb 1, 2025
5 of 7 checks passed
@jmr
Copy link
Member

jmr commented Feb 1, 2025

Thanks!

@jmr
Copy link
Member

jmr commented Feb 1, 2025

macos-latest times out: https://github.com/google/s2geometry/actions/runs/13091503445/job/36528655440

This should not take 30 minutes, the other runners are much faster.

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.

4 participants