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

do PR validation on Travis-CI (or GitHub Actions?) instead of Jenkins #507

Closed
SethTisue opened this issue May 11, 2018 · 20 comments
Closed

Comments

@SethTisue
Copy link
Member

SethTisue commented May 11, 2018

the groundwork for this is already laid.

the main blocker had been that partest took too long but that's fixed now, so we'll be well under the 90 minute limit the Travis-CI folks have kindly granted us.

@SethTisue

This comment has been minimized.

@SethTisue

This comment has been minimized.

@SethTisue
Copy link
Member Author

my initial stabs at this tried to share code with the full bootstrap-and-publish job, but the bootstrap stuff is so hairy (for both inherent and historical/accidental/fixable reasons), it's going to easier to work separately from that first, then once it's working, consider removing duplication. (Lukas agrees.)

the nicest experience will be achieved if we split up the parts of testAll so they all run in parallel on Travis and produces a separate logfile. but for starters I'm just trying to run testAll and get that working.

@SethTisue
Copy link
Member Author

we can't publish the PR snapshots to Artifactory because pull request jobs on Travis-CI don't have access to secure environment variables

I suggest we:

  • use publishLocal to do PR validation itself
  • worry separately about publishing to Artifactory
    • probably by adding a new /publish command to Scabot (usable only by committers) that would push the PR branch to scala/scala (under some standardized naming scheme like PR/1234) at which point the existing publish stuff would take over. (this would have the advantage of publishing fully bootstrapped rather than mini-bootstrapped artifacts)

@SethTisue
Copy link
Member Author

cleaner PR at scala/scala#6630. uses publishLocal for the mini-bootstrap; doesn't otherwise publish.

SethTisue added a commit to SethTisue/scala that referenced this issue May 12, 2018
use only sbt, avoid using external shell scripts and environment
variables.  we just need a few simple commands right here in
.travis.yml

doesn't publish anything to Artifactory.  let's discuss the way
forward on that at scala/scala-dev#507
@SethTisue
Copy link
Member Author

SethTisue commented May 15, 2018

I hit merge on scala/scala#6630, so remaining todos include:

  • let both forms of PR validation operate in parallel for a while and see how the Travis side does, in particular whether it catches the same failures Jenkins catches
  • see if we can cut the runtime back, perhaps by:
    • paying for premium instances
    • parallelizing the different parts of testing, by having a "build" stage copy files to S3 (Lukas has ideas on how to accomplish this securely without secrets, which PR jobs can't have) first
  • figure out a way to publish to scala-pr-validation-snapshots from Travis-CI
    • Lukas has ideas, probably involving an entirely separate job/repo, or something? can't recall details
    • Seth feels like adding an explicit /publish command to Scabot might be adequate, no strong feeling though
  • make appropriate changes to Scabot
  • integrate better with the new GitHub "checks" stuff
  • eventually disable the Jenkins jobs, remove the Jenkins scripts from the scripts directory in the scala/scala repo, and perhaps shut down a behemoth or two

@SethTisue SethTisue removed their assignment Apr 6, 2019
@SethTisue
Copy link
Member Author

we haven't been super happy with Travis-CI for PR validation. timeouts, subpar UI for log browsing and analysis, ...

the publishing-from-Travis aspect has worked fine, but with a big exception, which is that we don't have a way to publish Scala versions for as-yet-unmerged PRs, because we don't have access to the necessary encrypted environment variables from PR runs. (whereas our Jenkins setup has those secrets, in a way we don't think any PR can inappropriately access.)

today, in 2019, GitHub Actions exists and seems worth considering as well.

@lrytz
Copy link
Member

lrytz commented Sep 26, 2019 via email

@martijnhoekstra
Copy link

martijnhoekstra commented Sep 27, 2019

I made a variation to run on github actions a small while back. It demonstrates running on a matrix of adoptopenjdk 8, 11 and 12 (and failing on everything but 8) https://github.com/martijnhoekstra/scala/runs/220138323

@SethTisue
Copy link
Member Author

SethTisue commented Feb 26, 2020

@smarter is pushing GitHub Actions. I think maybe he gets a commission 💰

but the Dotty folks don't have a unmerged-PR-publishing solution, scala/scala3#6145 remains open. regardless, we can surely learn from what their team is doing

@SethTisue
Copy link
Member Author

SethTisue commented Feb 27, 2020

as we discussed in scala/contributors Gitter today, note that Travis-CI and Jenkins test different things. Jenkins tests the PR's branch HEAD as-is. Travis-CI tests the merge commit. (though by the time we actually hit "merge", HEAD of the target branch may already have moved on, so that result may be stale.)

there are pros and cons to both.

note also that with Travis-CI you get different behavior depending on whether the PR comes from a fork or not. if the PR comes from a branch in the same repo, you get both the branch run and the merged run. but in scala/scala all PRs always come from forks. so the branch run only occurs if Travis-CI has explicitly been enabled in the fork. (except, gah, at present in scala/scala that branch run will always fail; see #663)

@dwijnand
Copy link
Member

(Moving from scala/scala#8731 (comment))

@lrytz

It's one-time only anyway, the travis build is not re-run as the target branch moves ahead.

True. I guess given enough merge conflicts, old branches will always have to be rebased before their PRs can be merged (relevant to me as I just finished rebasing scala/scala#5892). But, otherwise, the risk here is that an old branch goes green on all its commits, but it then break once merged (semantic conflicts).

Makes me wonder if there's a relationship between how verbose/terse a language is and how frequent projects in those languages get merge conflicts. Did bors come out of c/c++/rust projects which had less merge conflicts but more semantic conflicts? 🤔

@lrytz
Copy link
Member

lrytz commented Feb 27, 2020

Semantic conflicts happen once in a while, recently scala/scala#8709

@dwijnand
Copy link
Member

Yeah, and those are more common than my PR-of-an-old-branch case.

@SethTisue SethTisue changed the title do PR validation on Travis-CI instead of Jenkins do PR validation on Travis-CI (or GitHub Actions?) instead of Jenkins Aug 18, 2020
@nogurenn
Copy link

nogurenn commented Aug 21, 2020

Is there a list of things somewhere I can check that the new tool must cover? Wanted to try this out for GitHub Actions

@SethTisue
Copy link
Member Author

SethTisue commented Aug 21, 2020

Is there a list of things somewhere I can check that the new tool must cover?

What Travis-CI currently does is in our .travis.yml, and what Jenkins does is in scripts/jobs/validate/test. There is some doc in README.md.

differences I'm aware of:

  • Windows testing is currently Jenkins-only (and uses scripts/jobs/integrate/windows, not scripts/jobs/validate/test, though the two scripts are broadly similar)
  • for no special reason, only Travis-CI currently does the extra Dotty-compatibility check sbt -Dscala.build.compileWithDotty=true library/compile
  • actual publishing of releases (including nightlies) is now only on Travis-CI
  • but only Jenkins publishes to scala-pr-validation-snapshots, as already discussed in comments above

in general, PR validation is easy-ish (the bootstrap aspect is only modestly tricky) and publishing is harder

There would be value even in just running the tests on GitHub Actions, especially if it worked on both Linux and Windows. Even if we can't retire the rest of Jenkins yet, it would be awfully nice if we could retire the Windows aspect of Jenkins (2.13, 2.12, 2.11). (At one point we were thinking of accomplishing that by adding an Appveyor build, but if GitHub Actions can do it, then great.)

If any solution can be used on all three branches (2.11.x, 2.12.x, 2.13.x), that'd be great. But there's value even in a solution that leaves out 2.11 (and/or 2.12). (For example, we didn't bother moving 2.11 release publishing off Jenkins, since there may never be another 2.11.x release.)

Wholesale changes to all of this can be daunting, but incremental change can have value too.

@SethTisue
Copy link
Member Author

parallelizing the different parts of testing, by having a "build" stage copy files to S3 (Lukas has ideas on how to accomplish this securely without secrets, which PR jobs can't have) first

Dale and I got this done recently using Travis-CI's new-ish "workspaces" feature.

@SethTisue
Copy link
Member Author

so the branch run only occurs if Travis-CI has explicitly been enabled in the fork. (except, gah, at present in scala/scala that branch run will always fail; see #663)

Dale fixed this recently, but the tests take long enough that it's of limited usefulness to contributors, who will use up their free allotment of minutes pretty darn fast, under Travis-CI's new limits.

@SethTisue
Copy link
Member Author

closing — #751 is the new ticket on this

@dwijnand
Copy link
Member

dwijnand commented Dec 4, 2020

Dale fixed this recently, but the tests take long enough that it's of limited usefulness to contributors, who will use up their free allotment of minutes pretty darn fast, under Travis-CI's new limits.

I've reverted back to travis-ci.org for the rest of the month... but I'll need a new idea in the new year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants