nrepl / piggieback

nREPL support for ClojureScript REPLs
480 stars 48 forks source link

Moving build to CircleCI #102

Closed shen-tian closed 5 years ago

shen-tian commented 5 years ago

First stab at moving the linter(s) and builds over to CircleCI. Notes

Does not do the codecov side, as I imagine you don't want both travis and CircleCI doing that? But that should be relatively easy to get.

Does not user Orbs, as I suspect that's better left to the stage when we have a lot of projects that need to share configs, and it's clear what's needed.

Edit: link on where my fork is building: https://circleci.com/gh/shen-tian/workflows/piggieback/tree/circleci

cichli commented 5 years ago

Thank you for looking at this! 🙌

Does not do the codecov side, as I imagine you don't want both travis and CircleCI doing that? But that should be relatively easy to get.

It can probably handle it okay. It merges multiple builds from the same CI provider – see here – it should do the same for multiple providers. The hit counts will of course be inflated but the coverage should still be measured correctly.

Does not user Orbs, as I suspect that's better left to the stage when we have a lot of projects that need to share configs, and it's clear what's needed.

Agreed.

@bbatsov you will need to activate CircleCI for this repo here.

shen-tian commented 5 years ago

A few other notes:

codecov-io commented 5 years ago

Codecov Report

Merging #102 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #102   +/-   ##
=======================================
  Coverage   85.78%   85.78%           
=======================================
  Files           1        1           
  Lines         190      190           
  Branches       11       11           
=======================================
  Hits          163      163           
  Misses         18       18           
  Partials        9        9

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7bcda97...cb6e6c8. Read the comment docs.

shen-tian commented 5 years ago

Added Cloverage/Codecov job. See results here: https://codecov.io/gh/shen-tian/piggieback/commits

plexus commented 5 years ago

The source for the orbs I've used is here: https://github.com/lambdaisland/meta/tree/master/circleci

The docker images used are created like this: https://nextjournal.com/plexus/openjdk11-%2B-clojure

shen-tian commented 5 years ago

The source for the orbs I've used is here: https://github.com/lambdaisland/meta/tree/master/circleci

The docker images used are created like this: https://nextjournal.com/plexus/openjdk11-%2B-clojure

Thanks! Will simplify a few things. @plexus: I haven't looked into it, but what does it take for an orb to be in the CircleCI repo?

@cichli : other than the CircleCI setup in the nrepl org, what do you feel we still need to do here?

shen-tian commented 5 years ago

I did some tidying up, with executors so that we don't have weird hanging - at end of jdk_versions. I didn't end up using @plexus's orbs, (after trying for a while), because there would have been a few fiddly bits to align on (running as root v.s. circleci etc.), which is not essential to this PR.

cichli commented 5 years ago

I will merge once we have a confirmed build on this repo. cc @bbatsov :-)

bbatsov commented 5 years ago

Sorry for the delay. I'll take a look over the weekend.

danielcompton commented 5 years ago

@shen-tian there are lots of small jobs and workflows here. In my experience, most of the time spent running tests is constant factors of pulling images, downloading dependencies, starting up containers, e.t.c. I wonder if you could get away with a smaller number of workflows, each doing more work in a single job?

The pattern I was imagining was to have one job per JDK version, and have each job do all of the work in one go, rather than splitting it up over multiple jobs.

Btw, I'm super impressed with what you've done here, very nice usage of the new CircleCI features, I especially liked the JDK version selection.

danielcompton commented 5 years ago

I've made a different branch which has two jobs:

https://circleci.com/gh/danielcompton/workflows/piggieback/tree/use-circle-ci-dc https://github.com/danielcompton/piggieback/blob/use-circle-ci-dc/.circleci/config.yml

The main benefit of these changes is that the CI config might be a little bit easier to follow. The downside is that the wall-clock time is a bit slower (4 mins instead of 3 mins), and some things are done multiple times unnecessarily, like eastwood and clfmt.

Edit: Generating combinatorial expansion of Lein test profiles brings the test time down to roughly 3-4 minutes for both jobs, although some of my perf improvements would also apply to this PR and could speed it up further.

shen-tian commented 5 years ago

@shen-tian there are lots of small jobs and workflows here. In my experience, most of the time spent running tests is constant factors of pulling images, downloading dependencies, starting up containers, e.t.c. I wonder if you could get away with a smaller number of workflows, each doing more work in a single job?

The pattern I was imagining was to have one job per JDK version, and have each job do all of the work in one go, rather than splitting it up over multiple jobs.

This actually came down less to how to get all the tests to run, but more showing, at a glance, which jobs in the build matrix failed. This is probably less important for piggieback, and more so for the likes on nRepl itself. Even here, we are testing across the combination of JDK versions, Clojure versions, and nRepl versions. It's thus useful to see, e.g. the build broke on JDK11 + Clojure 1.9 and nRepl 0.6, as an example. Given the assumption that's a desirable thing (is it? I'm inferring this from how the Travis build is set up) the maintainers would know), having high granularity at the jobs level would be useful.

Screenshot 2019-03-11 at 08 24 24

Re optimising CircleCI jobs for runtime, I've definitely found, as you point out, that pulling images takes up 20s or more, and penalises otherwise clever looking many-small-job workflows. The current setup is definitely not optimising for run time.

I realise we are testing all nRepl versions via the makefiles, so we are not actually splitting those out into the separate jobs. I've got a separate question about having some of the project complexity spread across project.clj Makefile and config.yml. It feels like more of what's in Makefile can be done in CircleCI than in Travis? And given the similarity between projects in the org, the CircleCI common patterns could be moved into Orbs. Then, each project would mostly declare what needs to be tested, rather than repeat the how part. But that's something that can be dealt with later if we like what we end up with here.

Steps forward: more input on the above welcome? Making any of the changes discussed should be quick/easy enough.

danielcompton commented 5 years ago

more showing, at a glance, which jobs in the build matrix failed.

Yep, this is a pretty good argument. I don't feel strongly about this, just wanted to test out an alternative, but it is nice being able to see the specific cases that fail at a glance.

bbatsov commented 5 years ago

Yeah, I agree. It's nice to be able to quickly figure out what exactly failed.

I realise we are testing all nRepl versions via the makefiles, so we are not actually splitting those out into the separate jobs. I've got a separate question about having some of the project complexity spread across project.clj Makefile and config.yml. It feels like more of what's in Makefile can be done in CircleCI than in Travis? And given the similarity between projects in the org, the CircleCI common patterns could be moved into Orbs. Then, each project would mostly declare what needs to be tested, rather than repeat the how part. But that's something that can be dealt with later if we like what we end up with here.

I think we've added the makefile mostly for the benefit of end users, who'd run some tasks manually, and for readability (as some commands had pretty long lein commands associated with them).

I don't even know what an orb is so I can't comment on this, but in general I can imagine that most projects would have similar builds.

shen-tian commented 5 years ago

Updated this to be in line with nrepl/nrepl#131. Managed to reuse a lot of config, so we are looking good for some Orbs later on.

Updated the badge and removed travis completely.

Might still need the project enabled on CircleCI

bbatsov commented 5 years ago

Thanks!