search

nim-lang / fusion

Fusion is for now an idea about how to grow Nim's ecosystem without the pain points of more traditional approaches.
MIT License
128 stars 16 forks source link

CI JS #64

Closed juancarlospaco closed 3 years ago

juancarlospaco commented 3 years ago
timotheecour commented 3 years ago

@juancarlospaco how about this: in fusion.nimble:

  exec "nim c -r src/fusion/docutils " & srcDir
=>
  exec "nim c -r src/fusion/docutils " & srcDir & " --outdir:htmldocs_js -d:fusionJsDocs"
  exec "nim c -r src/fusion/docutils " & srcDir & " --outdir:htmldocs"

and then handle when defined(fusionJsDocs) properly in docutils.nim

=> then we end up with 2 top-level dirs for docs: a js-only one a regular backend-only one

juancarlospaco commented 3 years ago

--styleCheck:error can not be for now, because theres stuff with broken style on Nim main repo on /lib/js/jsffi.nim and /lib/std/private/*.nim.

timotheecour commented 3 years ago

because theres stuff with broken style on Nim main repo on /lib/js/jsffi.nim and /lib/std/private/*.nim.

I see, you're hitting https://github.com/nim-lang/Nim/issues/10201

theres stuff with broken style on Nim main repo

=> https://github.com/timotheecour/Nim/issues/64#issuecomment-753691214

juancarlospaco commented 3 years ago

Can not reproduce, also works on the CI.

temp

timotheecour commented 3 years ago

@juancarlospaco as i predicted in https://github.com/nim-lang/fusion/pull/64#discussion_r551053905, passing explicitly -d:js is incorrect.

reduced case for the error i had in https://github.com/nim-lang/fusion/pull/64#discussion_r551068377 with nimble docs:

echo "discard" > z03.nim XDG_CONFIG_HOME= nim doc -b:js -d:js z03.nim

/Users/timothee/git_clone/nim/Nim_devel/lib/system/jssys.nim(54, 1) Error: redefinition of 'getCurrentException'; previous declaration here: /Users/timothee/git_clone/nim/Nim_devel/lib/system.nim(2386, 8)

devel a0134671eeed3cfac1e9035568cbdec41825da8d: error 1.4.0: no error (might explain why you didn't get an error)

basically, in this case, it causes issues because of interaction of -d:js and nimscript (but there could be other bad consequences for -d:js being passed explicitly)

note

I'm not sure why CI doesn't show an error here (maybe another bug that hides this bug; we should investigate whether fusion CI actually also runs on latest nim devel instead of a cached version or a nightly), but the fix here is:

exec "nim c -r -d:fusionDocJs src/fusion/docutils " & srcDir & " -d:js -d:fusionDocJs"
=>
exec "nim c -r -d:fusionDocJs src/fusion/docutils " & srcDir
juancarlospaco commented 3 years ago

I can revert tests/tmatching.nim but CI wont be green, you literally passed the code to workaround it. :shrug:

timotheecour commented 3 years ago

but CI wont be green

but I'm trying to understand why is CI green currently (before your PR) ?

juancarlospaco commented 3 years ago

You already seen the test that wont pass with specific versions, etc. I am just trying to make it more strict, cover JS too, even tried to enforce style. :)

If you think this makes CI worse, feel free to close it.

timotheecour commented 3 years ago

feel free to close it.

your PR is good, I'm just trying to understand that specific point; I'll do some experiments in https://github.com/nim-lang/fusion/pull/66 to understand what's going on with tests/tmatching.nim

timotheecour commented 3 years ago

indeed, I was right to be suspicious about the changes to tests/tmatching.nim; see https://github.com/nim-lang/fusion/issues/68 and https://github.com/nim-lang/fusion/pull/67; they shouldn't be needed (some changes that fix style are ok though)

EDIT: ok, https://github.com/nim-lang/fusion/pull/67 was merged, let's try removing the changes to tests/tmatching.nim (minus style fix ones if you want)

juancarlospaco commented 3 years ago

@timotheecour ping. Also https://github.com/nim-lang/Nim/pull/16630#issuecomment-756668463