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

Add GitHub Actions CI builds #183

Merged
merged 1 commit into from
Dec 1, 2020
Merged

Add GitHub Actions CI builds #183

merged 1 commit into from
Dec 1, 2020

Conversation

jidicula
Copy link
Contributor

@jidicula jidicula commented Nov 29, 2020

Checklist

  • I've given this PR a concise, self-descriptive, and meaningful title
  • I've linked relevant issues in the PR body
  • I've applied the relevant labels to this PR
  • I've added relevant tests for my contribution
  • I've updated the documentation and/or added correct docstrings
  • I've assigned a reviewer

Description

Why this change was necessary
Travis doesn't offer great support for macOS and is also beginning to
charge for macOS build minutes.

What this change does

  • Refactors the Travis CI build config into a shell script for unit tests
  • Calls that shell script in a GitHub Actions Workflow after setting up dependencies
  • Adds a matrix build in the GH Actions Workflow for macOS 10.15
    Catalina and 11.0 Big Sur (on x86_64 Intel chips), and Ubuntu 18, 20, and
    16
  • Adds a matrix build in the GH Actions Workflow for Python 3.7 and
    3.8
  • Adds a build status badge to the README

Additional context/notes/links
https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing

Linked issues

Resolves #180

@jidicula jidicula added testing Related to the testing process card:TO_REVIEW Assigned PR under review labels Nov 29, 2020
@jidicula jidicula marked this pull request as draft November 29, 2020 02:09
@jidicula jidicula force-pushed the ji/issue-180 branch 3 times, most recently from e427d61 to f0ab33a Compare November 29, 2020 02:22
@jidicula jidicula removed the request for review from jcohenadad November 29, 2020 02:23
@jidicula jidicula force-pushed the ji/issue-180 branch 4 times, most recently from 5994ca1 to 6fe852e Compare November 29, 2020 03:02
@jidicula jidicula marked this pull request as ready for review November 29, 2020 03:06
@jidicula jidicula force-pushed the ji/issue-180 branch 2 times, most recently from d157b55 to 1dda881 Compare November 29, 2020 03:14
@jidicula
Copy link
Contributor Author

jidicula commented Nov 29, 2020

@jcohenadad I think it's good to go now. All the GH Actions runs are passing (love those green checkmarks 😁) and the Python version build matrix looks fine too!

Note that I've set up Coveralls to only run on the Ubuntu 18.04 build. The tiny decrease in coverage looks like it's only on Python 3.8 (did some detective-work with which build finished first and matched it with the reports on coveralls.io). There might be a way to do some kind of averaging over all the build matrix OS options but I need to look into it further – I think it's best to handle that in a different PR.

The Coveralls builds run over all matrix builds now and I believe it averages coverage over each build parameter. 3.8 seems to have fewer lines run under the hood, giving it the appearance of having lower coverage. However, since the build matrix will be running in every PR CI check from now on, it's really just setting a new baseline coverage percentage that future PRs will be measured against.

Copy link
Member

@po09i po09i left a comment

Choose a reason for hiding this comment

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

I looked more closely at the builds and even though they say they pass, they all have errors within the CI run. I think its related to prelude and dcm2niix not being installed.

@jidicula
Copy link
Contributor Author

I looked more closely at the builds and even though they say they pass, they all have errors within the CI run. I think its related to prelude and dcm2niix not being installed.

Oh good catch - maybe a ci.sh isn't such a great idea since it obscures this stuff... I'll try to fix this up

jidicula added a commit that referenced this pull request Nov 29, 2020
Resolves #183 (review)
Related to #180

**Why this change was necessary**
The CI script was soldiering through without throwing an error code
when individual steps failed. Additionally, the steps for dcm2niix
installation did not work on the macOS builds.

**What this change does**
 - Adds early exits to script if steps fail.
 - Sets up OS-dependent steps for dcm2niix installation
jidicula added a commit that referenced this pull request Nov 30, 2020
Resolves #183 (review)
Related to #180

**Why this change was necessary**
The CI script was soldiering through without throwing an error code
when individual steps failed. Additionally, the steps for dcm2niix
installation did not work on the macOS builds.

**What this change does**
 - Adds early exits to script if steps fail.
 - Sets up OS-dependent steps for dcm2niix installation
jidicula added a commit that referenced this pull request Nov 30, 2020
Resolves #183 (review)
Related to #180

**Why this change was necessary**
The CI script was soldiering through without throwing an error code
when individual steps failed. Additionally, the steps for dcm2niix
installation did not work on the macOS builds.

**What this change does**
 - Adds early exits to script if steps fail.
 - Sets up OS-dependent steps for dcm2niix installation
@jidicula
Copy link
Contributor Author

@po09i @jcohenadad I narrowed down the failing build to the macOS build needing the macOS prelude binary. I would like to add the macOS prelude binary to the binaries repo, then conditionally set up some steps in the GH Actions workflow to use the correct binary depending on what build matrix OS option is running. Could you give me push access to the binaries repo?

jidicula added a commit to shimming-toolbox/binaries that referenced this pull request Nov 30, 2020
Related to shimming-toolbox/shimming-toolbox#183 (review)

**Why this change was necessary**
Now that shimming-toolbox is including macOS in its CI, a different
binary is needed for macOS CI testing.

**What this change does**
Adds a macOS prelude binary.
jidicula added a commit to shimming-toolbox/binaries that referenced this pull request Nov 30, 2020
Related to shimming-toolbox/shimming-toolbox#183 (review)

**Why this change was necessary**
Now that shimming-toolbox is including macOS in its CI, a different
binary is needed for macOS CI testing.

**What this change does**
Adds a macOS prelude binary.
@jidicula jidicula requested a review from po09i November 30, 2020 03:24
Copy link
Member

@po09i po09i left a comment

Choose a reason for hiding this comment

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

LGTM!

ci.sh Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
ci.sh Outdated Show resolved Hide resolved
@jidicula jidicula force-pushed the ji/issue-180 branch 4 times, most recently from cc17ac3 to 007bac1 Compare December 1, 2020 02:34
Resolves #180

**Why this change was necessary**
Travis doesn't offer great support for macOS and is also beginning to
charge for macOS build minutes[1].

**What this change does**
 - Factors the Travis CI build config into a GH Actions Workflow
 - Adds a matrix build in the GH Actions Workflow for macOS 10.15
   Catalina and 11.0 Big Sur (on x86_64 Intel chips), and Ubuntu 18,
   20, and 16
 - Adds a matrix build in the GH Actions Workflow for Python 3.7 and
 3.8
 - Adds a build status badge to the README
 - Adds Coveralls parallel build coverage metrics[2]

**Additional context/notes/links**
[1] https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing
[2] https://coveralls-python.readthedocs.io/en/latest/usage/configuration.html#github-actions-gotcha
@jidicula jidicula merged commit 243eab9 into master Dec 1, 2020
@jidicula jidicula deleted the ji/issue-180 branch December 1, 2020 02:41
@jidicula jidicula removed the card:TO_REVIEW Assigned PR under review label Dec 1, 2020
kousu pushed a commit that referenced this pull request Mar 20, 2021
Resolves #180

**Why this change was necessary**
Travis doesn't offer great support for macOS and is also beginning to
charge for macOS build minutes[1].

**What this change does**
 - Factors the Travis CI build config into a GH Actions Workflow
 - Adds a matrix build in the GH Actions Workflow for macOS 10.15
   Catalina and 11.0 Big Sur (on x86_64 Intel chips), and Ubuntu 18,
   20, and 16
 - Adds a matrix build in the GH Actions Workflow for Python 3.7 and
 3.8
 - Adds a build status badge to the README
 - Adds Coveralls parallel build coverage metrics[2]

**Additional context/notes/links**
[1] https://blog.travis-ci.com/2020-11-02-travis-ci-new-billing
[2] https://coveralls-python.readthedocs.io/en/latest/usage/configuration.html#github-actions-gotcha
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Related to the testing process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for macOS on Travis CI
3 participants