Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI JS #64

Merged
merged 48 commits into from
Jan 8, 2021
Merged

CI JS #64

merged 48 commits into from
Jan 8, 2021

Conversation

juancarlospaco
Copy link
Contributor

@juancarlospaco juancarlospaco commented Jan 3, 2021

@juancarlospaco juancarlospaco changed the title Add division by zero, fixes DivByZeroError CI JS WIP Jan 3, 2021
@juancarlospaco juancarlospaco marked this pull request as ready for review January 3, 2021 16:58
@juancarlospaco juancarlospaco marked this pull request as draft January 3, 2021 19:40
@timotheecour
Copy link
Member

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 nim-lang/Nim#10201

theres stuff with broken style on Nim main repo

=> timotheecour/Nim#64 (comment)

@juancarlospaco
Copy link
Contributor Author

Can not reproduce, also works on the CI.

temp

@timotheecour
Copy link
Member

timotheecour commented Jan 4, 2021

@juancarlospaco
as i predicted in #64 (comment), passing explicitly -d:js is incorrect.

reduced case for the error i had in #64 (comment) 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

Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't quite understand the changes in tests/tmatching.nim ; didn't CI pass before your PR ?

also links like https://github.com/nim-lang/fusion/pull/64/checks?check_run_id=1640671490#step:8:251 expire (CI keeps those for limited time only) so these should point to somewhere else, for example:

  • copy paste the relevant part of logs into a conversation comment in this PR (showing the source)
  • link to those instead (would look like: CI JS #64 (comment))

@juancarlospaco
Copy link
Contributor Author

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

@timotheecour
Copy link
Member

but CI wont be green

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

@juancarlospaco
Copy link
Contributor Author

juancarlospaco commented Jan 4, 2021

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
Copy link
Member

feel free to close it.

your PR is good, I'm just trying to understand that specific point; I'll do some experiments in #66 to understand what's going on with tests/tmatching.nim

@timotheecour
Copy link
Member

timotheecour commented Jan 4, 2021

indeed, I was right to be suspicious about the changes to tests/tmatching.nim; see #68 and #67; they shouldn't be needed
(some changes that fix style are ok though)

EDIT: ok, #67 was merged, let's try removing the changes to tests/tmatching.nim (minus style fix ones if you want)

@juancarlospaco
Copy link
Contributor Author

@timotheecour ping. Also nim-lang/Nim#16630 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants