refs #8638 nimble-wide CI: first pass #10247

timotheecour · 2019-01-09T13:48:19Z

refs [proposal] run whitelist of popular nimble packages as a part of non-blocking CI suite, to catch regressions early #8638 nimble-wide CI: first pass
[EDIT] the extra computation time spent seems ok for 20 packages; we could scale up

~~TODO: need to figure how to run nimble test~~ DONE

example output

see from https://travis-ci.com/timotheecour/Nim/jobs/169254994

TestResultAll:
i:    0  pkg: jester          seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 0  totalTime: 4.386720180511475
i:    1  pkg: cligen          seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 1  totalTime: 1.467899084091187
i:    2  pkg: libffi          seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 0  totalTime: 0.9745161533355713
i:    3  pkg: glob            seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 1  totalTime: 5.504637002944946
i:    4  pkg: nimongo         seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 1  totalTime: 15.49166083335876
i:    5  pkg: nimx            seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 1  totalTime: 5.231872081756592
i:    6  pkg: karax           seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 1 testOK: 1  totalTime: 18.29054808616638
i:    7  pkg: freeimage       seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 0  totalTime: 1.256640911102295
i:    8  pkg: regex           seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 0  totalTime: 4.114385843276978
i:    9  pkg: nimpy           seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 0  totalTime: 4.289348840713501
i:    10 pkg: zero_functional seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 1  totalTime: 1.651534080505371
i:    11 pkg: arraymancer     seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 0  totalTime: 8.910984039306641
i:    12 pkg: inim            seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 1 testOK: 1  totalTime: 8.552016973495483
i:    13 pkg: c2nim           seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 1 testOK: 0  totalTime: 13.97467684745789
i:    14 pkg: sdl1            seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 0  totalTime: 0.9655599594116211
i:    15 pkg: iterutils       seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 1  totalTime: 2.912926912307739
i:    16 pkg: gnuplot         seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 1  totalTime: 2.922753095626831
i:    17 pkg: nimpb           seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 1 testOK: 1  totalTime: 10.72133898735046
i:    18 pkg: lazy            seenOK: 1  foundOK: 1  cloneOK: 1  installOK: 1  developOK: 0 buildOK: 0 testOK: 0  totalTime: 0.9004640579223633
i:    19 pkg: choosenim       seenOK: 1  foundOK: 0  cloneOK: 0  installOK: 0  developOK: 0 buildOK: 0 testOK: 0  totalTime: 5.960464477539062e-06
TOTAL 20                      seenOK: 20 foundOK: 19 cloneOK: 19 installOK: 19 developOK: 0 buildOK: 4 testOK: 10 totalTime: 112.5204899311066

for future PR's

TOOD: figure out a better way to notify about test results (shouldn't be blocking a PR so shouldn't make travis fail, yet should be visible easily), and especially if previously succeeding tests are now failing

[EDIT] potential concerns to think about down the line

( this makes appveyor timeout, but travis is fine; maybe we can have a when defined(windows) block to have a shorter list of pkgs to test on appveyor
see https://ci.appveyor.com/project/Araq/nim/builds/21497722/messages
Build execution time has reached the maximum allowed time for your plan (90 minutes).
it went up to 3/20 packages

not clear whether order in which packages are tested matters, eg if they have side effects

andreaferretti · 2019-01-09T16:34:25Z

Caveat: nimble install does not test library compilation. What nimble install does for most libraries (certainly for all libraries of mine) is just to clone the project and copy the contents to the .nimble folder. This is not the right approach to test that compilation still works for anything.

nimble test makes sense, but even then, only for libraries that implement that task.

andreaferretti · 2019-01-09T16:41:56Z

Moreover, libraries can have custom requirements for testing:

maybe they are platform specific
maybe they need some specific dependency to be installed
maybe they need specific hardware (e.g. a GPU)

Only the author knows this, and so the CI job to test a specific library should be in charge of that author. I do not see a robust way to do a global CI infrastructure without letting library authors register and submit packages to Nim CI manually. This will also ensure that CI runs for libraries that are actively maintained, and if there is a problem with a library (say the library has to be updated to comply with some Nim changes), there is a registered user to contact

timotheecour · 2019-01-10T01:07:43Z

Caveat: nimble install does not test library compilation. What nimble install does for most libraries (certainly for all libraries of mine) is just to clone the project and copy the contents to the .nimble folder. This is not the right approach to test that compilation still works for anything.

yup, fixed by s/kCompileFail/kInstallFail/ ; note that it does test compilation works for certain packages (eg ones with bin specified, eg https://github.com/nim-lang/c2nim/blob/master/c2nim.nimble#L8)

~~I'll see whether it makes sense to run nimble build (or nim c foo.nim for a package foo ) (perhaps in some cases this doesn't make sense, idk yet)~~
EDIT: latest push now also runs nimble build for the build

For your other points:

I want something that works as best effort and leverages nimble wide standards for how to test packages; there's a very big value in standardization.
If a package doesn't provide nimble test, too bad, we can ping the package author or simply report nimble test un-implemented in package directory, so that it'll get eventually fixed for ppl who care about having their package not broken by future Nim versions

Moreover, libraries can have custom requirements for testing:

I have a simple answer to that, which again leverages nimble-wide standardization. /cc @dom96

introduce a standard nimble testCI task with following semantic:
- if testCI is in nimble tasks, nim's CI runs nimble testCI and that's expected to succeed (errors will get escalated, maybe blocking a nim PR that makes it fail)
- else, nim's CI runs nimble test (and failures are reported on best effort, but not escalated)

nimble testCI, when defined, is meant to be run from a CI framework such as Nim's ; it allows a package author to write arbitrary logic as you suggest, eg: platform specific, add necessary install / test steps (eg:
nimterop defines installWithDeps https://github.com/genotrance/nimterop/blob/master/nimterop.nimble#L27 which installs extra dependencies); in its case it'd look like:

task testCI:
  installWithDepsTask() # see https://github.com/nim-lang/nimble/issues/176 (run task from other task)
  testTask()
  # arbitrary logic can be added by pkg author, eg: `when defined(windows)`, when defined(appveyor)`, querying for GPU etc

all that Nim's runCI does is run nimble testCI or nimble test (in addition to stuff I'm already running eg build,install)

…le build` for each pkg

* show runtime for each pkg * cleanups

Araq · 2019-01-10T10:15:28Z

tools/nimbleci.nim

+  if b.stats.testOK == 0:
+    a.failures.add b.pkg
+  macro domixin(s: static[string]): untyped = parseStmt(s)
+  for k, v in fieldPairs(b.stats):


Suggested change

for k, v in fieldPairs(b.stats):

for aa, bb in fields(a.stats, b.stats):

aa += bb

Araq · 2019-01-10T10:16:20Z

tools/nimbleci.nim

+    # pid useful to kill process, since because of a bug, ^C doesn't work inside exec
+
+  var data: TestResult
+  let pkgs0 = """


This should be a const and moved to the top of the file.

dom96

This is a nice idea but I'm rejecting it because I don't think it belongs in Nim's test suite.

This should be a separate effort that gets executed every x hours/days, not for every push/PR on the Nim repo. Put this in another repo please and maintain it separately. Flag up problems that you're seeing to us. Doing it in this repo will be far too noisy.

Araq · 2019-01-13T09:58:27Z

@dom96 We need it and if it causes problems wrt timeouts etc we'll move it around, but as a first step we try to run it for every PR.

narimiran · 2019-01-13T10:35:23Z

This should be a separate effort that gets executed every x hours/days, not for every push/PR on the Nim repo. Put this in another repo please and maintain it separately.

I agree with @dom96, on both counts: it should be a separate repo/CI, and it should run once per day (not for every push).

alehander92 · 2019-01-13T13:41:49Z

Well, running it for each PR(at least when it's in "finished" state) it one of the main points: running it once a day doesn't solve this, except if you run it once a day for each PR with new commits(but even then you need to wait before merging small PRs)

andreaferretti · 2019-01-13T18:58:17Z

@timotheecour Unless the user is root on CI, I don't see how one can run - for instance - apt-get install openblas (or are we on OSX, then it is brew. Do we have to keep this logic in the nimble file?). If the project requires a GPU, well, I don't see a way to tell the CI engine to provision the CI machine with one. Finally, I don't see a way to tell to only run the CI on certain platforms

Araq · 2019-02-05T14:30:47Z

I took over here: #10549

Here is how it looks: https://ci.appveyor.com/project/Araq/nim/builds/22121288/job/0ujefy265yelp09c/tests

timotheecour force-pushed the exp_8638_ci_nimble branch from 3ffa632 to a920bef Compare January 9, 2019 13:49

timotheecour changed the title ~~refs #8638 nimble-wide CI: first pass~~ [WIP] refs #8638 nimble-wide CI: first pass Jan 9, 2019

timotheecour added the CI label Jan 9, 2019

timotheecour requested a review from Araq January 9, 2019 14:08

timotheecour force-pushed the exp_8638_ci_nimble branch 4 times, most recently from 5452d4f to 1a357a3 Compare January 9, 2019 15:47

timotheecour changed the title ~~[WIP] refs #8638 nimble-wide CI: first pass~~ refs #8638 nimble-wide CI: first pass Jan 9, 2019

timotheecour force-pushed the exp_8638_ci_nimble branch from a1f8958 to df17d2d Compare January 9, 2019 16:08

timotheecour force-pushed the exp_8638_ci_nimble branch from df17d2d to 6a5e7d3 Compare January 10, 2019 03:52

timotheecour added 4 commits January 10, 2019 01:49

refs nim-lang#8638 nimble-wide CI: first pass

fb8211b

CI tests now run nimble test; better error reporting

b148788

refactor nimbleci into separate standalone module, and also run `nimb…

a0efd9a

…le build` for each pkg

* format nimble CI results using aligned table

2c58644

* show runtime for each pkg * cleanups

timotheecour force-pushed the exp_8638_ci_nimble branch from 6a5e7d3 to 2c58644 Compare January 10, 2019 09:54

Araq reviewed Jan 10, 2019

View reviewed changes

timotheecour mentioned this pull request Jan 12, 2019

OpenSSL error breaking nimble and every package #10281

Closed

dom96 requested changes Jan 12, 2019

View reviewed changes

timotheecour mentioned this pull request Jan 16, 2019

wrong context shown for Error: can raise an unlisted exception #10325

Closed

Araq closed this Feb 5, 2019

timotheecour added the TODO items remaining label Feb 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refs #8638 nimble-wide CI: first pass #10247

refs #8638 nimble-wide CI: first pass #10247

timotheecour commented Jan 9, 2019 •

edited

Loading

andreaferretti commented Jan 9, 2019 •

edited

Loading

andreaferretti commented Jan 9, 2019

timotheecour commented Jan 10, 2019 •

edited

Loading

Araq Jan 10, 2019

Araq Jan 10, 2019

dom96 left a comment

Araq commented Jan 13, 2019

narimiran commented Jan 13, 2019

alehander92 commented Jan 13, 2019 •

edited

Loading

andreaferretti commented Jan 13, 2019

Araq commented Feb 5, 2019

-  for k, v in fieldPairs(b.stats):
+  for aa, bb in fields(a.stats, b.stats):
+    aa += bb

refs #8638 nimble-wide CI: first pass #10247

refs #8638 nimble-wide CI: first pass #10247

Conversation

timotheecour commented Jan 9, 2019 • edited Loading

example output

for future PR's

[EDIT] potential concerns to think about down the line

andreaferretti commented Jan 9, 2019 • edited Loading

andreaferretti commented Jan 9, 2019

timotheecour commented Jan 10, 2019 • edited Loading

Araq Jan 10, 2019

Choose a reason for hiding this comment

Araq Jan 10, 2019

Choose a reason for hiding this comment

dom96 left a comment

Choose a reason for hiding this comment

Araq commented Jan 13, 2019

narimiran commented Jan 13, 2019

alehander92 commented Jan 13, 2019 • edited Loading

andreaferretti commented Jan 13, 2019

Araq commented Feb 5, 2019

timotheecour commented Jan 9, 2019 •

edited

Loading

andreaferretti commented Jan 9, 2019 •

edited

Loading

timotheecour commented Jan 10, 2019 •

edited

Loading

alehander92 commented Jan 13, 2019 •

edited

Loading