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

switch to github actions (take two) #1568

Merged
merged 3 commits into from
Dec 4, 2019
Merged

switch to github actions (take two) #1568

merged 3 commits into from
Dec 4, 2019

Conversation

reaperhulk
Copy link
Contributor

This mostly just lifts and shifts us to GitHub Actions, but there are a few other changes:

  • I made a physical file for the C binary that gets compiled for run_simple.py because EOF echoing requires hard tabs and that doesn't play nicely with yaml files.
  • The tests will now run on a daily basis even if nothing is merged (this is useful to detect changes that result in failure on unpinned deps)
  • We only run CI on master branch now, so if you want to see if your branch works (and you're not operating off a fork) you'll need to submit a PR. But really, just develop on a branch on your own fork 😊
  • There is now a lint extra in setup.py
  • Try out codecov forr coverage data. Code Climate's other features aren't particularly useful and it doesn't surface coverage data easily. Plus that s3 upload thing, boo hiss.
  • We can probably significantly simplify the bash for this, but I want to keep the changes reviewable so that's a challenge for another day.

@ehennenfent
Copy link
Contributor

ehennenfent commented Dec 2, 2019

Like:

  • Switching away from CodeClimate is fine with me. Black and mypy catch almost all of the issues that it would flag, and not being able to see or download coverage files for individual branches has been very frustrating.
  • It should be very simple to split the symbolic WASM tests from the concrete tests, which should shave ~8 minutes off of the total time taken by the WASM tests (See: generate_tests.sh)

No like:

  • Since we're hard-coding the versions of black and mypy in setup.py, we should have a separate lint_dependencies list that we copy into the appropriate places rather than duplicating those entries.
  • It looks like CodeCov is only taking the last coverage file uploaded, not combining them.

@ehennenfent
Copy link
Contributor

ehennenfent commented Dec 2, 2019

I'd also suggest updating the CI badge in the README

Codecov
[![Codecov](https://img.shields.io/codecov/c/github/trailofbits/manticore)](https://codecov.io/github/trailofbits/manticore)

Edit: once this PR is merged, I think this should work:
GitHub Workflow Status
![GitHub Workflow Status](https://img.shields.io/github/workflow/status/trailofbits/manticore/ci)
Note that it's currently missing the hyperlink to the actual CI page since I'm uncertain of the URL.

@reaperhulk
Copy link
Contributor Author

Thanks for the feedback @ehennenfent! I updated the PR to add the badges and fix the lint deps. codecov should automatically merge coverage, but it's possible that it's misbehaving since it normally wants information about coverage on the master branch before it starts providing statistics.

Once this is merged I'll look at splitting up the wasm tests to get a bit more speed as well :)

@ehennenfent
Copy link
Contributor

One more thing: Looking over the workflow file, I think these are branch builds, not merged builds? If we're using these to check PR's, the first step should probably be to merge into master and confirm there are no conflicts (and that the tests will pass once merged).

@reaperhulk
Copy link
Contributor Author

The PR builds check against a merge commit, not the PR head. You can see the way it does this in the test output here: https://github.com/trailofbits/manticore/pull/1568/checks?check_run_id=330553477#step:2:727

Copy link
Contributor

@ehennenfent ehennenfent left a comment

Choose a reason for hiding this comment

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

Aha! So we've got:

  • Lint Deps
  • CI Badges
  • confirmed merge commit

I think we can go ahead and merge this. We should check that the CI badges and coverage measurements are working correctly once we have the actions running on master.

@ehennenfent ehennenfent merged commit 6b77189 into master Dec 4, 2019
@ehennenfent ehennenfent deleted the reaperhulk-patch-1 branch December 4, 2019 17:30
ekilmer added a commit that referenced this pull request Dec 17, 2019
* master: (33 commits)
  removes travis entirely (#1572)
  Run mypy on manticore directory by default (#1573)
  Fix deprecation warnings related to setting verbosity (#1574)
  Bump version to fix pip install on Python 3.6 (#1571)
  Direct CI badge directly to CI action (#1570)
  switch to github actions (take two) (#1568)
  Add --quick-mode to configure Manticore for fast exploration (#1555)
  pytest-xdist prevents disturbing stdout, revert to travis_wait (#1566)
  Show black diff on travis builds (#1565)
  various changes to resolve test timeout/invalid dep issues (#1563)
  try pytest xdist (#1561)
  switch to pytest (#1560)
  Fixes different problems found by lgtm (#1558)
  reorg some deps a bit, usee env markers for dataclasses (#1559)
  fix some deprecation warnings (#1556)
  Add flag to only generate alive states when finalizing Manticore (#1554)
  Arrayvarname (#1552)
  Fix Z3 version parsing (#1551)
  Update README.md (#1553)
  Manticore 0.3.2 (#1547)
  ...
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.

2 participants