`nimdigger`: build nim at any revision since v0.12.0~157, 1 liner `git bisect` to help find regressions by timotheecour · Pull Request #18119 · nim-lang/Nim

timotheecour · 2021-05-28T22:51:04Z

nimdigger is a tool to:

build nim at any revision (including custom branches), taking
care of details such as figuring out automatically the correct csources/csources_v1 revision to use.
allow 1-liner git bisect to find nim regressions, take care of building nim (if not cached) at intermediate revisions

design goals

ease of use: 1 liner for running git bisect workflows, or to build nim at past revisions
performance: via caching both csources built binaries, and intermediate nim binaries
lazyness: build artifacts on demand
go as far back as possible, currently oldest buildable nim version is v0.12.0~157
isolated: all artifacts are kept under a (configurable) $HOME/.nimdigger/cache/Nim

examples

build at any revision >= v0.12.0~157

$ nim r tools/nimdigger.nim --compileNim --rev:v0.15.2~10
$ $HOME/.nimdigger/cache/Nim/bin/nim -v
Nim Compiler Version 0.15.2 (2021-05-28) [MacOSX: amd64] [...]

find a which commit introduced a regression

$ nim r tools/nimdigger.nim --oldnew:v0.19.0..v0.20.0 --bisectCmd:'bin/nim -v | grep 0.19.0'
66c0f7c3fb214485ca6cfd799af6e50798fcdf6d is the first REGRESSION commit

find a which commit introduced a bugfix

$ nim r tools/nimdigger.nim --oldnew:v0.19.0..v0.20.0 --bisectBugfix --bisectCmd:'bin/nim -v | grep 0.20.0'
be9c38d2659496f918fb39e129b9b5b055eafd88 is the first BUGFIX commit

actual examples

for regression #16376:
copy this snippet to /tmp/t16376.nim:

type Matrix[T] = object
  data: T
proc randMatrix*[T](m, n: int, max: T): Matrix[T] = discard
proc randMatrix*[T](m, n: int, x: Slice[T]): Matrix[T] = discard
template randMatrix*[T](m, n: int): Matrix[T] = randMatrix[T](m, n, T(1.0))
let B = randMatrix[float32](20, 10)

$ nim r tools/nimdigger.nim --oldnew:v0.19.0..v0.20.0 -- bin/nim c --hints:off --skipparentcfg --skipusercfg /tmp/t16376.nim
fd16875561634e3ef24072631cf85eeead6213f2 is the first REGRESSION commit

for regression #16496:

$ nim r tools/nimdigger.nim --oldnew:v1.2.0..v1.4.0 -- bin/nim c -r -d:case2 --hints:off --skipparentcfg --skipusercfg /tmp/t16376.nim
8b66412a8bfebcd53f09d0e608b418a05d13516a is the first REGRESSION commit

for regression #18113:

$ nim r tools/nimdigger.nim --oldnew:v1.2.0..origin/devel -- bin/nim c -r -d:case3 --hints:off --skipparentcfg --skipusercfg /tmp/t16376.nim
e5873b3a9300897443f0f2e2db128e3463089003 is the first REGRESSION commit

speed

the more you use the tool, the faster it gets because binaries get cached, eg re-running this takes 10 seconds:

nim r tools/nimdigger.nim --oldnew:v1.2.0..origin/devel -- bin/nim c -r -d:case3 --hints:off --skipparentcfg --skipusercfg /tmp/t16376.nim

links

fixes nim-digger: program to build nim at any revision (including forks/PRs) (cf D's digger); useful for git bisect timotheecour/Nim#332
fixes ./koch bisect timotheecour/Nim#702
addresses 1st part of bisect bot juancarlospaco/nimbug#8
analog to D's https://github.com/CyberShadow/Digger

future work

enable running a command at specified set of past revisions (eg a whole range) to plot performance benchmarks across time (refs: add performance regression tests that can be tracked over git commit history timotheecour/Nim#425)
nimdustmite (refs [TODO] nim-dustmite source code minimization tool for automated bug reduction #8276) for bug minimization, analog to D's excellent https://github.com/CyberShadow/DustMite
machine readable output: --dump:output.json (refs nimdigger: build nim at any revision since v0.12.0~157, 1 liner git bisect to help find regressions #18119 (comment))

Varriount

This looks like a really useful tool! The source code could use some polishing though.

In particular, I think some identifiers could be changed to make the code more readable, and certain functions could be split up to increase code organization.

Varriount · 2021-05-29T03:03:02Z

+  result = outp
+  stripLineEnd(result)
+
+macro ctor(obj: untyped, a: varargs[untyped]): untyped =


I feel that this is unnecessary - the amount of typing it saves is minimal compared to the decreased readability and increased mental parsing required. Also, it's only used once in the entire file.

I'm also using this in nim-lang/fusion#32 and intend to add it to std/sugar in subsequent PR which will refactor its different uses.

The name should be sugar.construct IMO.

I still feel that the tradeoff between writeability and readability limits its usefulness though. In the cases where it would be useful (types with only a few fields) the fact that it's a macro significantly limits its usefulness - the reader must mentally adjust their expectations when they see the macro being used. In cases where there are types with many fields, it's almost always a better idea to explicitly show which values are being used for each field, as the reader is less likely to know the order of the fields.

I don't understand this remark but we can discuss this in an RFC about how we should have sugar.construct.

renamed to construct, RFC left TODO

Varriount · 2021-05-29T03:04:41Z

+    let make = "gmake"
+  else:
+    let make = "make"


Does this handle Windows systems?

i don't have windows; can you please try?

note that azure-pipelines.yml runs on windows and calls nimBuildCsourcesIfNeeded which calls this, so with the right setup, this should work on windows; a windows user will have to confirm though (and future work by a windows user can improve windows support)

This will work if the user is using WSL (which they may not be, if they are using Nim to build Windows executables).

so please try on windows and report back with how to improve windows support :)

Varriount · 2021-05-29T03:51:31Z

+    if opt.bisectBugfix:
+      msg2 = fmt"! ({msg2})" # negate exit code
+    let bisectCmd2 = fmt"{exe} --compileNim && ( {msg2} )"
+    runCmd(fmt"git -C {state.nimDir.quoteShell} bisect run bash -c {bisectCmd2.quoteShell}")


This is not cross-platform. Git should be able to run the command without using a shell.

can't windows subsystem for linux work here? (as done with azure-pipelines.yml on windows) Otherwise, I think this can be addressed in followup PR by people more familiar with windows (unless you have a concrete suggestion that makes it work for windows)

Varriount · 2021-05-29T03:56:08Z

Some other thoughts on this:

I think the logic handling cloning the csources and Nim repositories is unnecessary. Users will likely have these repositories already cloned.
Cache data should be stored in a temporary directory (in order to check):
- the directory in the XDG_CACHE_HOME environment variable,
- /tmp on *nix, or MacOS.
- %TEMP% on Windows (I think there's getTemporaryDirectory function somewhere?).
Is recompiling the executable necessary?

timotheecour · 2021-05-29T09:51:10Z

I think the logic handling cloning the csources and Nim repositories is unnecessary. Users will likely have these repositories already cloned.

It's necessary; an important design goal for nimdigger is isolation; it allows checking out revisions/bisecting without impacting your own git repo; it's much cleaner and safer to have nimdigger manage its own Nim/csources/csources_v1 repo (that can be wiped if needed) rather than reuse a users Nim checkout (which typically won't care about csources anymore since newer nim versions rely on csources_v1, not csources). Less risk of user data corruption etc.

Cache data should be stored in a temporary directory (in order to check):

as I mentioned in #18119 (comment), this should be handled instead as an API added to std/os which will offer a platform-independent way to access XDG_CACHE_HOME (as we already do with XDG_DATA_HOME via getConfigDir and TMP via getTempDir), instead of having tools like nimdigger redefine it. Once std/os adds this API, nimdigger will reuse it (separation of concerns). => timotheecour#744

Is recompiling the executable necessary?

since #17829, nim r caches binaries so it doesn't actually recompile; of course, nim c --outdir:bin tools/nimdigger is always possible

dom96

Is there a reason this needs to be in the Nim repo? I would push back against including it here and instead having it as its own standalone Nimble package.

Araq · 2021-05-31T09:42:28Z

I will test it on Windows. Not sure whether it should be a Nimble package or not, but it probably should. Nim core developers can be expected to type nimble install nimdigger or koch nimdigger, it should be mentioned in Nim's internal documentation.

juancarlospaco · 2021-05-31T09:45:08Z

Fusion ?. 🤔

Varriount · 2021-05-31T23:49:45Z

After some thought, I do agree with Araq that this should be a separate, installable utility.

dom96 · 2021-06-01T17:40:55Z

@juancarlospaco why? It's trivial to install via Nimble and if for some reason it's not then we should fix Nimble.

I think we should close this, this utility doesn't need to be in the same repo as the compiler.

timotheecour · 2021-06-01T20:31:07Z

I'm ok with a separate nimble package and linking to it in internal docs, I'll first address remaining comments and then update this PR accordingly

narimiran · 2021-06-02T05:55:44Z

+1 for this being a standalone nimble package.

timotheecour · 2021-06-03T23:35:07Z

@Araq done with all comments, now working on moving this to a separate repo;

how about fusion/tools/nimdigger? the goal being to avoid multiplying repos (fewer repos has benefits eg code reuse, less boilerplate wrt testing, doc publishing, more potential contributors etc); note that fusion/tools/nimdigger is intended to also be usable as a library

this would then be runnable via:

nim r /pathto/fusion/tools/nimdigger.nim --compileNim --rev:v0.15.2~10
# in future work, `nim r pkg/fusion/tools/nimdigger` could be supported,
# allowing passing canonical module paths instead of having to pass actual paths)

or if nimble build accepts building multiple binaries (does it?) then it could also be usable via nimdigger --compileNim --rev:v0.15.2~10

this assumes ppl are ok with allowing fusion to have some modules with these dependencies:

nim devel (instead of 1.0)
cligen

if not, then i'll just make a separate nimble package for simplicity

note

I expect other similar tools to be added in future, eg nimdustmite (#8276)

Araq · 2021-06-04T06:00:51Z

if not, then i'll just make a separate nimble package for simplicity

Separate Nimble package please but feel free to name the nimble package in preparation for much more code/utilities of yours. We both agree on the value mono-repos.

…hable untracked files

timotheecour marked this pull request as ready for review May 28, 2021 23:02

This was referenced May 28, 2021

choosenim devel --latest fails build dom96/choosenim#256

Closed

bisect bot juancarlospaco/nimbug#8

Open

timotheecour changed the title ~~nimdigger: build nim at any revision since v0.12.0~157, 1 liner git bisect to help find regressions~~ nimdigger: build nim at any revision since v0.12.0~157, 1 liner git bisect to help find regressions May 28, 2021

juancarlospaco reviewed May 29, 2021

View reviewed changes

Comment thread tools/nimdigger.nim

juancarlospaco reviewed May 29, 2021

View reviewed changes

Comment thread tools/nimdigger.nim

Varriount requested changes May 29, 2021

View reviewed changes

Varriount reviewed May 29, 2021

View reviewed changes

timotheecour mentioned this pull request May 29, 2021

add os.getDataDir for platform independent XDG_DATA_HOME timotheecour/Nim#744

Open

dom96 requested changes May 29, 2021

View reviewed changes

timotheecour force-pushed the pr_nimdigger branch from 22e210c to 3d00bfb Compare May 29, 2021 16:48

timotheecour mentioned this pull request May 30, 2021

add os.getCacheDir #18126

Merged

timotheecour force-pushed the pr_nimdigger branch from 3d00bfb to 7c3f0e6 Compare June 3, 2021 23:05

timotheecour added 7 commits June 4, 2021 13:04

add tools/digger

923ec24

bugfix

c119e0e

cleanup

137a5d0

DiggerState

7a20ac3

improve usage

3a28fd3

_

a5a73b6

add gitCleanDanger to avoid git bisect aborting when there are tras…

238720d

…hable untracked files

timotheecour added 16 commits June 4, 2021 13:04

improve doc

87c96de

remove timn/dgs

ad7d95e

fixup

63ff4a4

build binaries with -d:release

602f307

fix: nim doc tools/nimdigger.nim

53ff747

tools now have their docs alongside stdlib, compiler

3c8fbbf

improve doc wording

c7f1732

fixup

b1585f5

msg2=>msg

f80b6ec

improve docs/naming a bit

550aec6

address a comment regarding csourcesRevs

e3c1f6a

fixup

7cc8d5c

simplify

f806b91

use strscans

5debdaa

ctor => construct

8cb3e43

use getCacheDir("nimdigger") instead of ~/.nimdigger/cache

bffca2e

timotheecour force-pushed the pr_nimdigger branch from 0776b78 to bffca2e Compare June 4, 2021 20:04

timotheecour closed this Jun 6, 2021

timotheecour mentioned this pull request Aug 16, 2021

improvements to addInt and $ for integer types #18592

Merged

timotheecour mentioned this pull request Sep 29, 2021

use csources_v1 repo dom96/choosenim#266

Closed

Uh oh!

Conversation

timotheecour commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

design goals

examples

actual examples

speed

links

future work

Uh oh!

Uh oh!

Uh oh!

Varriount left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Varriount Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timotheecour May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timotheecour May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Varriount commented May 29, 2021

Uh oh!

timotheecour commented May 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dom96 left a comment

Choose a reason for hiding this comment

Uh oh!

Araq commented May 31, 2021

Uh oh!

juancarlospaco commented May 31, 2021

Uh oh!

Varriount commented May 31, 2021

Uh oh!

dom96 commented Jun 1, 2021

Uh oh!

timotheecour commented Jun 1, 2021

Uh oh!

narimiran commented Jun 2, 2021

Uh oh!

timotheecour commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

note

Uh oh!

timotheecour commented May 28, 2021 •

edited

Loading

Varriount Jun 1, 2021 •

edited

Loading

timotheecour May 29, 2021 •

edited

Loading

timotheecour May 29, 2021 •

edited

Loading

timotheecour commented May 29, 2021 •

edited

Loading

timotheecour commented Jun 3, 2021 •

edited

Loading