Post Mortem for {.experimental: "ForLoopMacros".}

[mratsim]

Post Mortem for {.experimental: "ForLoopMacros".}

Before I start, this first post will focus on facts. All opinions part will be cleanly separated.

Also the feature is called ForLoopStmt but its flag is called ForLoopMacros.

Context

Starting from Friday August, 17 continuous integration (CI) of Nimbus started to fail.

Subsequently while trying fixes in decreasing order of desiredness, several discussions
were sparked within the team regarding reliance on unstable features and simplification of the code.

History:

• 2018-04-15: Araq introduces for loop macro (ForLoopStmt) to Nim devel - nim-lang/Nim@c08efb4
• 2018-06-17: mratsim uses ForLoopStmt in Stint - Uint - allow compile-time evaluation for all procs nim-stint#54
  • PR allowed compile-time evaluation of all uint
    • This enabled the use of const across all EVM constants
    • As a side benefit, func/noSideEffect to detect reliance on global/shared state is easier as global const
    • As a side benefit, significant code saving were achieved while improving significantly readability and maintainability, PR is +180/-336 on code size
      • Note that we can remove the ForLoopStmt, retain functionality and space-saving at the cost of internal ergonomics/readability
    • As a side-benefit, benchmark were improved by 15% reaching TTMath speed without assembly thanks to full compile-time evaluation and loop unrolling.
• 2018-08-15: Araq gates all new features including ForLoopStmt behind the new {.experimental: "Flag".} pragma - Uint - allow compile-time evaluation for all procs nim-stint#54
  • Note that {.experimental: "notnil"} was also introduced
  • Activating the feature can be done with the following 2 ways:
    • Using {. experimental: "Foo" .} where the feature is used
    • Passing --experimental:Foo as a command-line (CLI) flag when compiling
• 2018-08-17: coffeepots PR has issue on Mac and Windows but succeed on Linux - More rpcs #108
• 2018-08-17: coffepots raises issue upstream Usability: {.experimental: “ForLoopMacros”.} is required at all macro callsites nim-lang/Nim#8676
  • mratsim detects the regression due to nim-lang/Nim@da41fc1
    • The change was announced on IRC and part of Nim changelog
    • Nimbus issue raised: {.experimental: "ForLoopMacros".} flag needed #109
• 2018-08-17: mratsim tries to implement a fix following the changelog instruction at the time
  • instructions: https://github.com/nim-lang/Nim/blob/3ca8ebe778cb82cdbd97d7a9a00015bbeed1a696/changelog.md
  • Fix: status-im/nim-stint@2152937
    • The fix was adding {.experimental: "ForLoopMacros".} where the macro was defined
  • Result: Failure to fix
• 2018-08-17: mratsim investigates in PR New {.experimental: "ForLoopMacros".} support nim-stint#62 and finds that
  • the experimental pragma must be added at the call site
    not at the macro definition site - Usability: {.experimental: “ForLoopMacros”.} is required at all macro callsites nim-lang/Nim#8676 (comment)
  • In stint all those macros are called via proc, so mratsim adds {.experimental: "ForLoopMacros".} everywhere: status-im/nim-stint@f0efc37
  • Result: still failure
  • Further investigation shows that for generic proc, you need the experimental pragma at call instantiation site
    due to generics early symbol resolution: [Meta] Generics/Static early symbol resolution nim-lang/Nim#8677 (meta issue raised during the investigation)
  • As a "final" solution PR New {.experimental: "ForLoopMacros".} support nim-stint#62
    • only reverts status-im/nim-stint@2152937
    • and add --experimental:ForLoopMacros to the nim.cfg
    • This works locally and on Linux but fails on Mac and 1+ hour later on Appveyor.
• 2018-08-20: mratsim further investigates in Don't use stint's nim.cfg during CI to compile koch nim-stint#63 and Add the new experimental forloopmacros flag for Stint fix #109 #110
  • In CI nimbus/nim.cfg config leaks into nimbus/Nim/ compilation options including csources and koch
  • This impacts Mac and Windows but not Linux due to the use of Docker there.
  • Trying to use --skipProjCfg --skipParentCfg create another error - --skipProjCfg --skipParentCfg do not work when compiling koch from csources nim-lang/Nim#8691 (comment)

    - cd ..
    - bin\nim --skipProjCfg --skipParentCfg c koch # Don't use stint's nim.cfg to compile koch
    - koch boot --skipProjCfg --skipParentCfg -d:release
    - koch --skipProjCfg --skipParentCfg nimble

  • In the end, mratsim comments out the nim.cfg and add the flag in nimble test task in Don't use stint's nim.cfg during CI to compile koch nim-stint#63
    • switch("experimental", "ForLoopMacros")
• 2018-08-21: coffeepots realizes that Appveyor tests are still failing
  • mratsim traces back to the use of nimble install -y instead of nimble install -dy in the Appveyor config
  • This implies that nimble tries to compile the dependencies it just installed
  • Unfortunately it does not use the --experimental flag in that case and Appveyor fails
  • Fix 986cba5

Differences between each "solutions"

In code

Due to nim-lang/Nim#8677, {.experimental: "ForLoopStmt"} is needed everywhere a Stint comparison is used.

This is an artifact of how experimental, macro call and generics early symbol resolution interact. Without any one of those elements,
{.experimental: "ForLoopStmt"} can be contained in Stint and wouldn't contaminate packages that depends on it like Nimbus

In nim.cfg

This means that locally devs can use nim c foo.nim and have the flag automatically turn on.

All nimble tasks would also picked this nim.cfg automatically.

Unfortunately, all libraries that depend on Stint would need to ship a nim.cfg

Since for CI, Nim compiler is bootstrapped as a subfolder of the project, it gets compiled with the project nim.cfg.
The current csources do not support the --experimental switch and CI fails (except Travis + Docker).

Araq mentionned the following solution on 2018-08-21 that mratsim did not see before writing the postmortem - nim-lang/Nim#8691 (comment)

@if nimHasForLoopMacros:
--experimental: forLoopMacros
@end

In nimbus.nimble

There is no contamination to Nim compilation.

All nimble tasks must add the switch.

Locally all devs must have their own nim.cfg, use nimble tasks (or vscode tasks) or nim --experimental:ForLoopStmt c foo.nim

CI must use the hidden undocumented -d flag when installing dependencies.
After installing, nimble tries to compile the code to produce a binary (even for libraries),
since this is done without the --experimental flag either in the code or via a nim.cfg, dependency installation fails.

[yglukhov]

Come on, this is just a nim bug.

More info: {.experimental: forLoopMacros.} needs only to be written in the module where the macro is defined.

[arnetheduck]

well, this certainly has lots of detail - kudos for that - good to have a trail always in case it comes up in discussion.

what's crucial about this type of events is that they break flow for the whole team, as more people get involved in "upgrading nim" than is necessary. we've talked about how to move forwards with this in the channel and elsewhere, so let's write it down here as well:

• we lock the Nim version in use and prefer officially release versions unless there are strong reasons to go with a patched one - this to make it easy to get up and running with nimbus development for anyone
• we use the branch from https://github.com/status-im/Nim for our own CI to increase trust in the CI pointing out failures in our code, not unrelated changes to Nim
• when it's time for upgrades, one person deals with testing nimbus for a new Nim version and making sure that all components still work as expected. this work needs to be timed cautiously so as not to break other in-progress branches. once fallout is taken care of and the new version works, we switch our Nim branch to that version.

[cheatfate]

Please consider that not only nimbus is affected there many repositories which needs to be tested too.