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 Testing #347

Merged
merged 8 commits into from
Jan 20, 2021
Merged

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Jan 5, 2021

PR checklist

  • Short (1 sentence) summary of your PR:
    Add Github Actions Testing on Pull Requests and Pushes to master and releases
  • Developer(s):
    apcraig, phil-blain
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.
    Only added yaml and supporting changes for Github Actions. Tested in Github Actions, https://github.com/apcraig/Icepack/actions
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

This is running the travis_suite via conda+macos on Github Actions. Initial implementation from @phil-blain and updated by @apcraig. This partly addresses CICE-Consortium/CICE#550. If we're happy with this, I'll do the same for CICE.

  • I added a badge to the README.md.
  • The travis-ci is still there as well, but isn't running now due to accounting issues at travis-ci
  • We will have to see how to add the Github Actions Testing to the PR Status Checks
  • We will have to see whether it all works and interacts properly in Github once merged
  • I tried to get the ubuntu-list working with conda+linux but could not. It seems the conda+linux does not include csh? I have never tested the conda+linux implementation before, does it really work? It might be worthwhile revisiting this question.

@apcraig
Copy link
Contributor Author

apcraig commented Jan 5, 2021

This has not triggered an action. It did when I was pushing to my development branch in my fork. Maybe this needs to be merged before it will trigger the action? Separately, we need to figure out whether something needs to be done to have the Testing Action be part of the PR status checks. Hopefully this all works once it's merged???

Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

I don't understand enough about how this works to intelligently review it. I'll approve so it can move forward but would appreciate more useful review by @phil-blain.

Where does the result of write_logfiles.csh get written?

Can we still use travis-ci to build the code, or will that need to be removed from README.md?

https://github.com/Apcraig/Icepack/actions appears to be hardwired. Will that stay this way? I see we're also using your space for lcov. That's fine, but we might want to think about making these local.

@apcraig
Copy link
Contributor Author

apcraig commented Jan 7, 2021

I'm planning to keep the travis-ci "on" for now. We can override the status checks and merge the PRs if needed. I'm still waiting to hear if travis gives us more time. Hopefully they will. If so, I think we should have both travis and github actions working concurrently for now.

All the output is written to the github actions log, it's very similar to what travis does. The logs are all visible. See here for an example of the github actions output,

https://github.com/apcraig/Icepack/runs/1653142536?check_suite_focus=true

@phil-blain
Copy link
Member

I won't have time to look at this today, but I'll try to tdo so on Monday or Tuesday.

@apcraig
Copy link
Contributor Author

apcraig commented Jan 18, 2021

@phil-blain, any chance to have a look?

Copy link
Member

@phil-blain phil-blain left a comment

Choose a reason for hiding this comment

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

I think this looks good, I left a few specific comments. I think it would be good to have linux working also, do you have an example of a failed run ? we could try to get it to work before we merge.

@apcraig
Copy link
Contributor Author

apcraig commented Jan 18, 2021

Turned linux on again, it still fails. @phil-blain, any ideas. It fails with

Run ln -s ${GITHUB_WORKSPACE}/../Icepack ${HOME}/icepack
ln -s ${GITHUB_WORKSPACE}/../Icepack ${HOME}/icepack
Error: Second path fragment must not be a drive or UNC name. (Parameter 'expression')

Under "link". In "system info", these env variables are written,

HOME: /home/runner
GITHUB_WORKSPACE: /home/runner/work/Icepack/Icepack

MacOS doesn't struggle with this. Unless we can fix this quickly, I'll just turn off linux again. MacOS seems to be working well and with the latest changes, it triggers the build as part of the PR which is great.

@phil-blain
Copy link
Member

This error is very weird as a Google search points to questions about the .NET framework and the C# programming language. After SSH-ing to the GitHub Actions virtual machine (see https://github.com/phil-blain/github-actions-debug/blob/main/.github/workflows/tmate.yml) and playing around, I found out that the command csharp launches a Mono C# Shell. So I think what happens is that since the default shell is configured in the workflow file to be "csh":

defaults:
  run:
    shell: /bin/csh {0}

but the Ubuntu images do not include csh, the GitHub Actions runner somehow matches "csh" to the command "csharp" and tries to run the ln command in the csharp shell, which fails.... This is obviously a bug in the Runner (see https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#custom-shell).

If we want to add Ubuntu (which I think could be a good idea) I think we should remove the "defaults" section that sets "csh" as the default shell and just use the default shell of the system, which is Bash. We would not have to then specify to use Bash for the other steps (see https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#using-a-specific-shell).

This way we test something that is closer to what our users are most likely to use. If we also want to test the whole thing in other shells like csh, then we would need to install it on ubuntu , using the instructions in our documentation:

# Install tcsh
sudo apt-get install tcsh
# Configure your system to use tcsh as csh
sudo update-alternatives --set csh /bin/tcsh

That could be done in a additional entry in the matrix.

But we can post-pone all that for later, and just disable Ubuntu for now.

@apcraig
Copy link
Contributor Author

apcraig commented Jan 19, 2021

Thanks @phil-blain, that's very helpful. I also was googling the ubuntu error when I was doing the original implementation and couldn't figure out what was happening. I also went back and forth on bash, csh, and zsh about which to use and how to implement it. I had some problems with bash as well. If I remember right, some of our scripts didn't work if I didn't use the csh shell. I'm sure that can be overcome somehow, I just couldn't figure it out. For now, I'll comment out again the ubuntu, verify it's working, then we can merge. I need to then do the same for CICE. The main thing is to get back our CI testing first. As you suggest, we can play with the ubuntu later. Thanks again.

@apcraig
Copy link
Contributor Author

apcraig commented Jan 19, 2021

OK, I think this may be ready to merge. @phil-blain, let me know if you think this is OK. I incorporated most of your comments. Thanks! Once this is merged, I'll setup the same for CICE and then we can decide later if we want to turn off the travis stuff. I still haven't heard anything from them. I'll ping them again today.

@apcraig apcraig merged commit 757cbcb into CICE-Consortium:master Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants