Skip to content

Conversation

@khansameer25
Copy link
Collaborator

Added GitHub Actions workflow for building and testing across Linux, macOS, and Windows. Configured jobs with caching for dependencies and build directories.

Copy link
Contributor

@kjohnsen kjohnsen left a comment

Choose a reason for hiding this comment

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

As in comments, I think we should use GitHub actions and pre-installed software where possible to make faster and easier to maintain. The current style writing the terminal commands ourselves does a better job of testing the installation process for the user, but our goal is to remove that need almost entirely.

if: runner.os != 'Windows'
run: |
if [ "${{ runner.os }}" == "macOS" ]; then
brew install cmake gfortran curl zip unzip gnu-tar
Copy link
Contributor

Choose a reason for hiding this comment

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

can we leave out gfortran and let vcpkg take care of it? we want to minimize these dependency installation steps that don't get cached

Copy link
Contributor

Choose a reason for hiding this comment

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

some of these tools come pre-installed and some have a convenient GitHub action to set up to speed up this step on all three OS's

Copy link
Contributor

Choose a reason for hiding this comment

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

For an example of pre-installed software available on Ubuntu: https://github.com/actions/runner-images/blob/main/images/ubuntu/toolsets/toolset-2404.json

Copy link
Contributor

Choose a reason for hiding this comment

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

git clone https://github.com/microsoft/vcpkg.git
cd vcpkg
./bootstrap-vcpkg.sh
./vcpkg integrate install
Copy link
Contributor

Choose a reason for hiding this comment

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

All these vcpkg steps can likely be taken care of for you by someone else:

Copy link
Contributor

@kjohnsen kjohnsen Feb 26, 2025

Choose a reason for hiding this comment

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

Very nice! The biggest question I have in my mind is what compilers are being used in the configure/build steps? One of the biggest headaches in getting the project to build right is that some compilers work and others don't for no apparent reason. Clang has worked most consistently across platforms, but not every version and not every machine we've tested...a mess. Assuming we will still have these problems, I think we will need to just choose a compiler, like Clang.

We may want to merge this now to get it working before adding more, but things we will want:

  • a static vs. dynamic option in the matrix
    • dynamic mostly to test install
    • static for goal of creating pre-compiled static binaries
  • config?
    • for static, easy-install binaries probably want both Release and RelWithDebInfo.
    • for dynamic probably just debug
  • deploying static binaries somewhere (probably GitHub releases?)

Then after getting the C++ build working:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The project uses default compilers: GCC on Linux, Clang on macOS, and MSVC on Windows, which are working fine so far. It's possible to specify Clang as the compiler for all platforms, but I'm unsure if it's necessary since the compiler is automatically selected based on the platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great---if default compilers are working let's just run with those

khansameer25 and others added 3 commits February 28, 2025 09:30
Change main to master

Co-authored-by: Kyle Johnsen <johnsenkyle13@gmail.com>
@khansameer25
Copy link
Collaborator Author

I made the requested changes regarding using pre-installed software and actions when possible, which helped clean up the code. I will create a new branch to work on the other features.

@khansameer25 khansameer25 requested a review from kjohnsen March 7, 2025 15:59
Copy link
Contributor

@kjohnsen kjohnsen left a comment

Choose a reason for hiding this comment

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

Pretty swell. Great work!

Copy link
Contributor

Choose a reason for hiding this comment

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

Great---if default compilers are working let's just run with those

run: |
if [ "${{ runner.os }}" == "macOS" ]; then
brew install cmake gfortran curl zip unzip gnu-tar
brew install cmake ninja
Copy link
Contributor

Choose a reason for hiding this comment

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

this step could be covered by the get-cmake action, but it's not a big enough deal to change it now

@khansameer25 khansameer25 merged commit f291e70 into master Mar 14, 2025
3 checks passed
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.

3 participants