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

[CI][1/2] Re-do the github actions workflows, migrate various travis and appveyor tests. #2675

Merged
merged 1 commit into from
May 26, 2021

Conversation

senhuang42
Copy link
Contributor

@senhuang42 senhuang42 commented May 19, 2021

This PR overhauls the CI into something hopefully a bit simpler/sensible. Now, we have recategorized the github actions into just two workflows: long and short tests. The short tests should provide quick feedback if there's some typo/build error.

All per-release tests on Travis have been changed to per-PR, and have either stayed in Travis or been moved to GH-actions if possible. Those remaining in travis typically are the more niche build configurations and whatnot. Adding a nightly build to travis as discussed offline isn't necessary since we can actually run these per-PR, since the remaining tests aren't so long (and Travis runs them in parallel, and completes faster than dev-long-tests).

Note that no tests have been removed, and tests run either as frequently or more frequently than before, though they may have been migrated or broken up into independent tests.

So, this PR:

  • Removes all existing gh actions workflows.
  • Adds dev-short-tests and dev-long-tests workflows. Short tests are defined as those that typically finish under 5min, long tests are typically more than 10 min.
  • Migrates as many of the travis and appveyor tests to gh actions.
  • Breaks up some of the really long tests that were basically multiple tests into individual jobs. Now the max length test is <40min (instead of 1hr+).
  • Adds memory sanitizer to oss-fuzz test.
  • Adds build-cancellation to redundant GH actions builds, to prevent long queues.

Next steps:

  • Migrate even more off of AppVeyor.

Open questions:

  • Should we still support visual studio 2013 and less? According to microsoft docs the 2013 visual studio "RTW" and below are no longer supported.
  • Should regressiontest get added as per-PR? I don't see why it'd make more sense to run nightly rather than per-PR. It's also not that long.
  • What to do regarding the newest clang static analyzer warnings? It seems like most of these are false positives.

@senhuang42 senhuang42 changed the title [CI] Re-do the github actions workflows, migrate various travis and appveyor tests. [CI][1/2] Re-do the github actions workflows, migrate various travis and appveyor tests. May 21, 2021
@Cyan4973
Copy link
Contributor

Should we still support visual studio 2013 and less? According to microsoft docs the 2013 visual studio "RTW" and below are no longer supported.

Our rule-of-thumb for compiler support is ~10 years.

So far, we have stopped supporting VC 2008.
We can add VC 2010 to this list.
VC 2013 should preferably remain supported up to ~2024.

@Cyan4973
Copy link
Contributor

Should regressiontest get added as per-PR? I don't see why it'd make more sense to run nightly rather than per-PR. It's also not that long.

If it's not long, then having it run per-PR seems to make sense.
I would like the point of view of @terrelln , which created this test.

What to do regarding the newest clang static analyzer warnings? It seems like most of these are false positives.

Is that transferred into #2677 ?

@senhuang42
Copy link
Contributor Author

senhuang42 commented May 21, 2021

If it's not long, then having it run per-PR seems to make sense.
I would like the point of view of @terrelln , which created this test.

It's under 20mn (faster than half of the dev-long-tests). Nick and I had briefly discussed this offline, and think it'd be a good idea to make it a per-PR test. I'll submit that as a separate PR though, since I'll have to test that the GH Actions artifact generation works as expected first.

Is that transferred into #2677 ?

Yeah, but I've left in the travis static analyze test in this PR, which uses an older version of clang and doesn't generate the warnings that cause the test to fail. So merging this PR still retains the static analyze test as-is.

@senhuang42 senhuang42 merged commit 414bcf2 into facebook:dev May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants