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 for Releases and a Docker container #6

Merged
merged 8 commits into from
Nov 19, 2021

Conversation

wfondrie
Copy link
Collaborator

@wfondrie wfondrie commented Nov 13, 2021

This PR refactors our previous GitHub Actions workflows to automatically upload build artifacts to releases created within GitHub. Here's how it should work:

When a commit is pushed to master or a pull-request is made with master as the target of the merge:

  1. Comet will be compiled on Ubuntu, MacOS, and Windows.
  2. We check that the new Comet executable can create a new param file. (Here is an excellent place for other tests if we have them)
  3. If any build / test fails, we should see an ❌ notifying us. If it's a pull-request we should probably fix the problem before merging it into master 😉.
  4. For the Linux build, we create a Docker container and verify that the Docker container works (per Docker container with releases #5).

When a new release is created using the "Releases" tab in GitHub:

  1. Comet will be compiled on Ubuntu, MacOS, and Windows.
  2. We check that the new Comet executable can create a new param file. Hopefully neither of these steps fail, because they should have completed successfully when the most recent commit was added to master.
  3. Each build will upload it's build artifacts to the Release page. Currently, I've configured these to be a compressed archive containing the executable and README.md for each platform. After all have completed you should see linux.tar.gz, macos.zip, and windows.zip available for download under the Releases tab.
  4. For the LInux build, we create a Docker container. This is uploaded to the GitHub Container Registry where folks can use Docker to grab it.

I'd advise on waiting to merge this PR until we have the Windows build up and running.

Any other feature requests, questions, or comments?

@jke000
Copy link
Collaborator

jke000 commented Nov 13, 2021 via email

@wfondrie
Copy link
Collaborator Author

Presumably add some file like RELEASENOTES.txt to the repo and copy that to each release archive?

This seems like a good idea to me. In my Python projects I use a file called CHANGELOG.md that contains the release history in the format suggested by https://keepachangelog.com/en/1.0.0/.

Regardless, we can put any files you want into the release archives.

What's the proper way to adding such tests and I guess specifically any advice on how I should deal with the spectra and sequences? Encode those within the program itself (not too horrible for the small tests) or can the data be external from the binary itself?

I'm certainly not an expert on testing C++ programs, but I know that the crux folks use Cucumber to run such tests (in crux they call these "smoke tests", but I think a lot of folks would call them "system tests". Alternatively I use pytest to run unit and system tests on all of my Python packages. If we just want to run Comet with different sets of parameters and compare the outputs, I could do this fairly easily with some Python code. Lastly, if we ever want to write unit tests (testing specific functionality within the code base), then GoogleTest is probably the most popular option (and the Percolator folks have started using it too).

As for where the tests go, they certainly do not have to be included with the executable. I think we would want to create a top level tests directory where the testing code, spectra, and sequences could reside. Additionally, if the spectra and/or sequences are too large to reasonably store in version control, we could also download them from somewhere before the tests are run.

In the case of crux, I know they keep small spectra and sequence files in the GitHub repo for testing.

@mammmals mammmals marked this pull request as ready for review November 19, 2021 23:01
Copy link
Collaborator

@mammmals mammmals left a comment

Choose a reason for hiding this comment

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

Looks good overall with the windows build holding us up for now.

Copy link
Collaborator

@mammmals mammmals left a comment

Choose a reason for hiding this comment

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

Approving to move forward

@mammmals mammmals merged commit fe0e4ef into master Nov 19, 2021
@mammmals mammmals deleted the more-actions branch November 19, 2021 23:05
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