-
Notifications
You must be signed in to change notification settings - Fork 5
GitHub Actions workflow for automated build and test #35
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
Conversation
kjohnsen
left a comment
There was a problem hiding this 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.
.github/workflows/build.yml
Outdated
| if: runner.os != 'Windows' | ||
| run: | | ||
| if [ "${{ runner.os }}" == "macOS" ]; then | ||
| brew install cmake gfortran curl zip unzip gnu-tar |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's recommended to use actions when possible. we might want to do this with things like clang and cmake build/configure:
.github/workflows/build.yml
Outdated
| git clone https://github.com/microsoft/vcpkg.git | ||
| cd vcpkg | ||
| ./bootstrap-vcpkg.sh | ||
| ./vcpkg integrate install |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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:
- steps for Python bindings: build, test, submit wheel to PyPI
- steps for Matlab mex extensions: build, test, deploy (also to GitHub releases?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Change main to master Co-authored-by: Kyle Johnsen <johnsenkyle13@gmail.com>
|
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. |
kjohnsen
left a comment
There was a problem hiding this 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!
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
Added GitHub Actions workflow for building and testing across Linux, macOS, and Windows. Configured jobs with caching for dependencies and build directories.