Adding devel to CI, fix #161, fix #149 #169

pietroppeter · 2023-02-01T08:55:05Z

I am adding both stable (that will become 2.x soon) and devel (which is candidate for 2.x). Also update versions of cache and setup-nim-action.

Done to check if recent toml fix makes nimib works in 2.0rc

We could also decide to expand the test matrix to multiple os.

fix #161 and #149

I am adding both stable (that will become 2.x soon) and devel (which is candidate for 2.x). Also update versions of cache and setup-nim-action

pietroppeter · 2023-02-01T09:04:22Z

we have a failure with devel but it seems about tsources (illegal storage access) and not about toml. not sure why the other two test runs have been canceled (edit: CI says The job was canceled because "devel" failed. we should probably have them run anyway there should be a github action setting for that)

see https://stackoverflow.com/a/65647509/4178189

pietroppeter · 2023-02-01T09:17:44Z

ok, now we have failure only in devel

ringabout · 2023-02-01T09:26:53Z

It seems to be a regression of ORC.

It doesn't work with ARC/ORC in 1.6.10 either.

ringabout · 2023-02-01T09:29:52Z

import nimib

nbInit
nbCode:
  # a comment
  let
    x = 1

C:\Users\blue\Desktop\Nim\compiler\nim.nim(167) nim
C:\Users\blue\Desktop\Nim\compiler\nim.nim(122) handleCmdLine
C:\Users\blue\Desktop\Nim\compiler\main.nim(307) mainCommand
C:\Users\blue\Desktop\Nim\compiler\main.nim(276) compileToBackend
C:\Users\blue\Desktop\Nim\compiler\main.nim(130) commandCompileToC
C:\Users\blue\Desktop\Nim\compiler\modules.nim(136) compileProject
C:\Users\blue\Desktop\Nim\compiler\modules.nim(56) compileModule
C:\Users\blue\Desktop\Nim\compiler\passes.nim(186) processModule
C:\Users\blue\Desktop\Nim\compiler\passes.nim(79) processTopLevelStmt
C:\Users\blue\Desktop\Nim\compiler\cgen.nim(2021) myProcess
C:\Users\blue\Desktop\Nim\compiler\cgen.nim(2015) genTopLevelStmt
C:\Users\blue\Desktop\Nim\compiler\cgen.nim(1064) genProcBody
C:\Users\blue\Desktop\Nim\compiler\ccgstmts.nim(1636) genStmts
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3071) expr
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(2765) genStmtList
C:\Users\blue\Desktop\Nim\compiler\ccgstmts.nim(1636) genStmts
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3121) expr
C:\Users\blue\Desktop\Nim\compiler\ccgstmts.nim(1271) genTryGoto
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3071) expr
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(2765) genStmtList
C:\Users\blue\Desktop\Nim\compiler\ccgstmts.nim(1636) genStmts
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3071) expr
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(2754) genStmtList
C:\Users\blue\Desktop\Nim\compiler\ccgstmts.nim(1636) genStmts
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3024) expr
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(830) genCall
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(828) genAsgnCall
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(429) genPrefixCall
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(407) genParams
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(312) genArg
C:\Users\blue\Desktop\Nim\compiler\cgen.nim(687) initLocExprSingleUse
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3030) expr
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(830) genCall
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(828) genAsgnCall
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(429) genPrefixCall
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(407) genParams
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(312) genArg
C:\Users\blue\Desktop\Nim\compiler\cgen.nim(687) initLocExprSingleUse
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3070) expr
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(2761) genStmtListExpr
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3070) expr
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(2754) genStmtListExpr
C:\Users\blue\Desktop\Nim\compiler\ccgstmts.nim(1636) genStmts
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3109) expr
C:\Users\blue\Desktop\Nim\compiler\ccgstmts.nim(1629) genAsgn
C:\Users\blue\Desktop\Nim\compiler\ccgstmts.nim(136) loadInto
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(828) genAsgnCall
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(429) genPrefixCall
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(407) genParams
C:\Users\blue\Desktop\Nim\compiler\ccgcalls.nim(294) genArg
C:\Users\blue\Desktop\Nim\compiler\cgen.nim(674) initLocExpr
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3052) expr
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(1579) genObjConstr
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(3009) expr
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(482) putDataIntoDest
C:\Users\blue\Desktop\Nim\compiler\ccgexprs.nim(323) genAssignment
C:\Users\blue\Desktop\Nim\compiler\ast.nim(1454) skipTypes
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

ringabout · 2023-02-01T10:21:44Z

macro getCodeAsInSource*(source: string, command: static string, body: untyped): string =
  ## Returns string for the code in body from source. 
  # substitute for `toStr` in blocks.nim
  let startPos = startPos(body)
  let filename = startPos.filename.newLit
  let endPos = finishPos(body)
  let endFilename = endPos.filename.newLit

  result = quote do:
    if `filename` notin nb.sourceFiles:
      nb.sourceFiles[`filename`] = readFile(`filename`)

    doAssert `endFilename` == `filename`, """
    Code from two different files were found in the same nbCode!
    If you want to mix code from different files in nbCode, use -d:nimibCodeFromAst instead. 
    If you are not mixing code from different files, please open an issue on nimib's Github with a minimal reproducible example."""

    getCodeBlock(nb.sourceFiles[`filename`], `command`, `startPos`, `endPos`)

It seems that in nimib/sources, the filename isn't semchecked properly, since it doesn't have a type at codegen phase.

pietroppeter · 2023-02-01T10:43:20Z

so that is something we recently touched in #163 and I think @HugoGranstrom is better positioned than me to answer about it.

HugoGranstrom · 2023-02-01T10:45:13Z

Isn't this just a bug in the compiler and not a problem on our side?

ringabout · 2023-02-01T10:45:37Z

Yeah, It's a weird compiler bug, though. I don't have a solution or a workaround.

HugoGranstrom · 2023-02-01T10:47:42Z

Ok, that's problematic 🤔 because we do need the filename and it must come from the macro. I'll have a look in the coming days and see if I can find an alternative way 👍

HugoGranstrom · 2023-02-02T21:37:09Z

I have tried to trick the compiler by wrapping the strung in a ident and running astToStr on the ident instead and some other tricks like that but to no avail. At the moment the only workaround I can think of is doing a staticWrite with the filename to a file at compile time and reading the file at runtime. But I would rather avoid that, and it might not even work 😅🤣

ringabout · 2023-02-03T02:09:04Z

You might report the bug to Nim if you happen to find a reduced case.

HugoGranstrom · 2023-02-03T15:58:55Z

I'll give it a try 👍

HugoGranstrom · 2023-02-03T17:39:59Z

I tried using nimib 0.3.0 and while it compiles it seems to have other problems with orc:

Traceback (most recent call last)
/home/hugo/.nimble/pkgs/nimib-0.3.0/nimib.nim(43) x_nim2
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/pure/os.nim(234) /
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/pure/os.nim(175) joinPath
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/pure/os.nim(143) joinPathImpl
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/pure/pathnorm.nim(98) addNormalizePath
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/system.nim(3071) substr
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/system/alloc.nim(908) alloc0
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/system/alloc.nim(905) alloc
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/system/alloc.nim(735) rawAlloc
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
Segmentation fault (core dumped)

So even if we manage to solve this specific issue I fear we have a few other orc bugs as well that isn't related to macros 🙃

HugoGranstrom · 2023-02-03T17:45:01Z

But for the macro, it seems like it is the startPos and endPos that it doesn't like. The filename works totally fine on its own.

HugoGranstrom · 2023-02-03T17:48:53Z

It seems to work if we create a literal of the Pos objects and pass them instead:

let startPosLit = startPos.newLit
let endPosLit = endPos.newLit

Then the question is whether the code without converting to a literal should work or not.

HugoGranstrom · 2023-02-03T17:51:53Z

@pietroppeter This is the new getCodeAsInSource:

macro getCodeAsInSource*(source: string, command: static string, body: untyped): string =
  ## Returns string for the code in body from source. 
  # substitute for `toStr` in blocks.nim
  let startPos = startPos(body)
  let filename = startPos.filename.newLit
  let endPos = finishPos(body)
  let endFilename = endPos.filename.newLit

  let endPosLit = endPos.newLit
  let startPosLit = startPos.newLit

  result = quote do:
    if `filename` notin nb.sourceFiles:
      nb.sourceFiles[`filename`] = readFile(`filename`)

    doAssert `endFilename` == `filename`, """
    Code from two different files were found in the same nbCode!
    If you want to mix code from different files in nbCode, use -d:nimibCodeFromAst instead. 
    If you are not mixing code from different files, please open an issue on nimib's Github with a minimal reproducible example."""

    getCodeBlock(nb.sourceFiles[`filename`], `command`, `startPosLit`, `endPosLit`)

Now I'm getting the same runtime error as I got in nimib 0.3.0 though...

HugoGranstrom · 2023-02-03T18:20:02Z

Issue for macro opened: nim-lang/Nim#21326

HugoGranstrom · 2023-02-03T21:56:21Z

I'm not able to wrap my head around the runtime error 😞 This reproduces it locally for me (I have installed #head of both toml_serialization and serialization):

import os
import "$nim/compiler/pathutils"

import nimib, nimib/[options, config]

var nb: NbDoc
nb.initDir = getCurrentDir().AbsoluteDir
loadOptions nb
loadCfg nb

let thisFile = "/home/hugo/code/nim/nimib/x_pathutils.nim".AbsoluteFile

echo thisFile
echo thisFile.splitFile
echo thisFile.splitFile.dir
echo thisFile.splitFile.dir.string / "a"

This gives this output:

[nimib] config file found: /home/hugo/code/nim/nimib/nimib.toml
/home/hugo/code/nim/nimib/x_pathutils.nim
(dir: /home/hugo/code/nim/nimib, name: "(dir: \x00\x00\x00ls", ext: ".nim")
/home/hugo/code/nim/nimib
Traceback (most recent call last)
/home/hugo/code/nim/nimib/x_pathutils.nim(16) x_pathutils
/home/hugo/.choosenim/toolchains/nim-1.6.10/compiler/pathutils.nim(30) splitFile
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/pure/os.nim(689) splitFile
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/system.nim(3071) substr
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/system/alloc.nim(908) alloc0
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/system/alloc.nim(905) alloc
/home/hugo/.choosenim/toolchains/nim-1.6.10/lib/system/alloc.nim(735) rawAlloc
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
Segmentation fault (core dumped)
Error: execution of an external program failed: '/home/hugo/.cache/nim/x_pathutils_d/x_pathutils_7DB931EC51DD8FF47DEDE3914D845AF3C1979994 '

And we can note several things. First, the name field of the splitFile looks really weird, but when printed on its own it looks ok. Second thing is that we are loading a nimib.toml file. If I remove it (or remove loadOptions or loadCfg) the program runs just fine. How could the toml file possibly affect the variable thisFile which has nothing to do with it?!?!

I have no clue what's going on and what to blame tbh. Is it toml_serialization with yet another orc bug or a bug in the compiler which just happens to show up when using toml_serialization for a totally different part of the program?

pietroppeter · 2023-02-04T07:43:42Z

This is getting really weird, did not have too much time to look into this myself and not sure how to help at the moment!

pietroppeter · 2023-02-04T07:46:22Z

I think anyway you did make a few good steps toward resolution, thanks for that! Last example with toml vs compiler bug I guess it could be nice to have it further simplified but it does not seem to be easy.

HugoGranstrom · 2023-02-04T11:03:49Z

It's definitely a toml problem, managed to reduce it to this:

import toml_serialization

type NbConfig* = object
  srcDir*, homeDir*: string

let f = readFile("nimib.toml")

let t = Toml.decode(f,  NbConfig, "nimib")

echo t # error here

# nimib.toml
[nimib]
srcDir = "docsrc"
homeDir = "docs"

So we are still blocked by toml_serialization. I'm honestly tempted to change toml library to https://github.com/NimParsers/parsetoml which I just tried and it works under orc. We don't need any fancy bells and whistles so using Status' libraries is overkill in my opinion. I might even go as far as to say that we should avoid using their libraries at all if we want our code to work on the latest versions of Nim in the future. Having to wait months for them to fix things is not viable... Then I prefer to use simpler libraries that don't try to do anything fancy with threads and pointers, they are less prone to break.

pietroppeter · 2023-02-04T13:57:18Z

Ok then we should report the error on toml serialization, that at least is good news since there is not anything weirder. I agree we should consider changing library, apart the various dependencies we get from status (which I never liked), it makes it hard to fix it ourselves. The api of parseToml though at the moment does not support how we use it in nimib (also nimibook will be affected). The advantage of toml serialization was that you could target parsing a specific section and parse using a type as specification, similar to what jsony does. It is possible we might be able to add that feature to parseToml, I will look into it.

HugoGranstrom · 2023-02-04T15:02:03Z

parsetoml has an open issue on parsing directly into an object. And a workaround is going through json (toml → json → nim). And as for the section, we just have to create a wrapper proc which checks for a nimib section in the file before trying to read from the section. So there isn't really any features that are missing, we just have to restructure our code a bit.

Have opened an issue on toml_serialization: status-im/nim-toml-serialization#62

pietroppeter · 2023-02-04T17:59:10Z

Do you want to give it a try?

HugoGranstrom · 2023-02-04T18:33:36Z

I don't have much free time this weekend but I could try it next weekend 👍

* fix orc macro bug * replace toml_serialization with parsetoml * clean up * use jsony for deserialzation as it can handle missing fields Co-authored-by: Hugo Granström <5092565+HugoGranstrom@users.noreply.github.com>

pietroppeter · 2023-02-09T10:26:39Z

should we bump to 0.4 or a minor bump to 0.3.6 given the parsetoml change? I would go with 0.3.6 but fine with the other too, @HugoGranstrom?

pietroppeter · 2023-02-09T10:32:45Z

and for the changelog I was thinking of bumping, releasing curating the automatic changelog and adding the results to changelog.md. We could actually change our workflow to "not update changelog during PRs" and only make sure that message in PR merge commit is a good for the automatic changelog provided by Github. I would still want for the moment to maintain the changelog.md (especially for the history part) by updating it after the release.

pietroppeter · 2023-02-09T10:34:23Z

ready to merge and release if you are ok with bumping to 0.3.6 @HugoGranstrom

HugoGranstrom · 2023-02-09T10:40:03Z

0.3.6 sounds good to me! 👌 Release it!

Changelog idea sounds good for the future 👍

Adding devel to CI

9c9e566

I am adding both stable (that will become 2.x soon) and devel (which is candidate for 2.x). Also update versions of cache and setup-nim-action

pietroppeter changed the title ~~Adding devel to CI~~ Adding devel to CI, fix #161 Feb 1, 2023

disable fail-fast for test CI

57458ca

see https://stackoverflow.com/a/65647509/4178189

pietroppeter mentioned this pull request Feb 1, 2023

add nimib to important packages nim-lang/Nim#20697

Merged

pietroppeter changed the title ~~Adding devel to CI, fix #161~~ Adding devel to CI, fix #161, fix #149 Feb 2, 2023

HugoGranstrom mentioned this pull request Feb 3, 2023

Orc compiler crash when not making object from lineInfoObj into a literal nim-lang/Nim#21326

Closed

HugoGranstrom mentioned this pull request Feb 8, 2023

Change toml lib + fix macro bug on devel #173

Merged

Change toml lib + fix macro bug on devel (#173) (#174)

a2bf928

* fix orc macro bug * replace toml_serialization with parsetoml * clean up * use jsony for deserialzation as it can handle missing fields Co-authored-by: Hugo Granström <5092565+HugoGranstrom@users.noreply.github.com>

bump to 0.3.6

a346167

pietroppeter merged commit e25ee44 into main Feb 9, 2023

pietroppeter mentioned this pull request Feb 13, 2023

Update existsFile and bump the version. pietroppeter/nimoji#15

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding devel to CI, fix #161, fix #149 #169

Adding devel to CI, fix #161, fix #149 #169

pietroppeter commented Feb 1, 2023 •

edited

pietroppeter commented Feb 1, 2023 •

edited

pietroppeter commented Feb 1, 2023

ringabout commented Feb 1, 2023 •

edited

ringabout commented Feb 1, 2023

ringabout commented Feb 1, 2023

pietroppeter commented Feb 1, 2023

HugoGranstrom commented Feb 1, 2023

ringabout commented Feb 1, 2023

HugoGranstrom commented Feb 1, 2023

HugoGranstrom commented Feb 2, 2023

ringabout commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023 •

edited

HugoGranstrom commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023

pietroppeter commented Feb 4, 2023

pietroppeter commented Feb 4, 2023

HugoGranstrom commented Feb 4, 2023

pietroppeter commented Feb 4, 2023

HugoGranstrom commented Feb 4, 2023

pietroppeter commented Feb 4, 2023

HugoGranstrom commented Feb 4, 2023

pietroppeter commented Feb 9, 2023

pietroppeter commented Feb 9, 2023

pietroppeter commented Feb 9, 2023

HugoGranstrom commented Feb 9, 2023

Adding devel to CI, fix #161, fix #149 #169

Adding devel to CI, fix #161, fix #149 #169

Conversation

pietroppeter commented Feb 1, 2023 • edited

pietroppeter commented Feb 1, 2023 • edited

pietroppeter commented Feb 1, 2023

ringabout commented Feb 1, 2023 • edited

ringabout commented Feb 1, 2023

ringabout commented Feb 1, 2023

pietroppeter commented Feb 1, 2023

HugoGranstrom commented Feb 1, 2023

ringabout commented Feb 1, 2023

HugoGranstrom commented Feb 1, 2023

HugoGranstrom commented Feb 2, 2023

ringabout commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023 • edited

HugoGranstrom commented Feb 3, 2023

HugoGranstrom commented Feb 3, 2023

pietroppeter commented Feb 4, 2023

pietroppeter commented Feb 4, 2023

HugoGranstrom commented Feb 4, 2023

pietroppeter commented Feb 4, 2023

HugoGranstrom commented Feb 4, 2023

pietroppeter commented Feb 4, 2023

HugoGranstrom commented Feb 4, 2023

pietroppeter commented Feb 9, 2023

pietroppeter commented Feb 9, 2023

pietroppeter commented Feb 9, 2023

HugoGranstrom commented Feb 9, 2023

pietroppeter commented Feb 1, 2023 •

edited

pietroppeter commented Feb 1, 2023 •

edited

ringabout commented Feb 1, 2023 •

edited

HugoGranstrom commented Feb 3, 2023 •

edited