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

Remove compiler and use nimscript with nim e instead #635

Closed
wants to merge 32 commits into from
Closed

Remove compiler and use nimscript with nim e instead #635

wants to merge 32 commits into from

Conversation

genotrance
Copy link
Contributor

  • Removed compiler dependency and use nim e instead
  • Added nimscriptwrapper.nim to replace nimscriptsupport.nim (still in source control)
  • Added OSX to CI
  • Added devel & stable to CI
  • Tests fixed to pass on Windows

@genotrance
Copy link
Contributor Author

cc @Araq @dom96

@Araq
Copy link
Member

Araq commented Apr 19, 2019

LGTM.

Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Nice job overall. I've got some questions and some changes I'd like you to make though.

nimble.nimble Outdated Show resolved Hide resolved
src/nimblepkg/nimscriptapi.nim Show resolved Hide resolved
src/nimblepkg/nimscriptapi.nim Show resolved Hide resolved
src/nimblepkg/nimscriptapi.nim Outdated Show resolved Hide resolved
src/nimblepkg/nimscriptapi.nim Show resolved Hide resolved
src/nimblepkg/nimscriptwrapper.nim Outdated Show resolved Hide resolved
src/nimblepkg/nimscriptwrapper.nim Outdated Show resolved Hide resolved
src/nimblepkg/nimscriptwrapper.nim Outdated Show resolved Hide resolved
src/nimblepkg/nimscriptwrapper.nim Outdated Show resolved Hide resolved
src/nimblepkg/nimscriptwrapper.nim Outdated Show resolved Hide resolved
@genotrance
Copy link
Contributor Author

I have made most of the changes as suggested. If there aren't any major concerns, I'd prefer merging this as is so that it can undergo some general testing by the community before 0.20 comes out.

Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

I think this is much better. There are still some things I would change but we can do that later.

There is one thing that I need you to fix though, please do so and then we can merge this.

cache:
directories:
- "$HOME/.nimble"
- "$HOME/.choosenim"
- "$HOME/.choosenim/toolchains/nim-0.19.4"
Copy link
Collaborator

@dom96 dom96 Apr 26, 2019

Choose a reason for hiding this comment

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

@genotrance as far as I'm concerned you need to revert this because it will break things, please do so (or explain why it must be this way).

nimble.nimble Show resolved Hide resolved
@genotrance
Copy link
Contributor Author

@genotrance as far as I'm concerned you need to revert this because it will break things, please do so (or explain why it must be this way).

See this job log for starters. Cache is restored but install is still rerun. Travis docs don't say that restoring cache will skip the install step, neither in the install nor cache documentation.

Even in nightlies we have to check for existence of nim binary to leverage caching.

Meanwhile, I tried caching the entire ~/.choosenim dir but ran into random Travis issues so backed that out for now. It can be done later as we get Windows CI working, once this PR is merged.

As for ~/.nimble, no doubt choosenim lives there but the redownload and nimble setup takes 1.2 seconds. It is likely it is already cached in the Travis http proxy. This way, we also get newer versions when they get released. Also, most tests are run against a custom nimbleDir but not generate so if for whatever reason that test fails and an older generate binary is in ~/.nimble/bin, the test might still pass.

Overall, caching isn't material to this PR. I will add Windows CI and ~/.choosenim caching separately. Please push this through and I can proceed onto other tasks.

@dom96
Copy link
Collaborator

dom96 commented Apr 28, 2019

Alright. I forgot that install was run anyway in Travis.

So I've checked out your branch and tested it. It looks like the caching mechanism doesn't work, it never gets invalidated. I tried to modify the Nimble file version from 0.9.0 to 0.10.0 and nimble dump keeps returning 0.9.0.

Then I removed the cache in /tmp/nimblecache and got this:

Screen Shot 2019-04-28 at 12 53 02

Seems there is still some work left to do :/ (That error message needs improvement too)

@genotrance
Copy link
Contributor Author

So I've checked out your branch and tested it. It looks like the caching mechanism doesn't work, it never gets invalidated. I tried to modify the Nimble file version from 0.9.0 to 0.10.0 and nimble dump keeps returning 0.9.0.

Looks like I broke cache checking when I re-architected this. I've put in a fix and added a test case as well.

Then I removed the cache in /tmp/nimblecache and got this:

I wasn't able to reproduce this.

dom96 added a commit that referenced this pull request Apr 29, 2019
Squashed commit of the following:

commit e86a376
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Sun Apr 28 15:37:22 2019 -0500

    Fix caching issue

commit 640ce3f
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Thu Apr 25 18:38:48 2019 -0500

    Clean up per feedback

commit ae3ef9f
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Thu Apr 25 16:39:26 2019 -0500

    Fix for 0.19.4

commit 915d6b2
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Thu Apr 25 16:13:42 2019 -0500

    Keep nimscript separate, pin devel

commit c278bd6
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Mon Apr 22 14:57:44 2019 -0500

    Hardcode version, json{}, code width 80, isScriptResultCached, no blank paramStr check

commit 64e5489
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Wed Apr 17 21:07:03 2019 -0500

    Remove compiler dependency

commit a031fff
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Wed Apr 17 16:49:09 2019 -0500

    Add devel to travis

commit d49916e
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Wed Apr 17 16:43:14 2019 -0500

    Interactive live, json to file

commit 24131de
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Wed Apr 17 12:40:27 2019 -0500

    Fix empty param, json echo

commit b22fe37
Merge: 5cf0240 2942f11
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Apr 16 22:23:17 2019 -0500

    Merge branch 'nocompiler' of https://github.com/genotrance/nimble into nocompiler

commit 5cf0240
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Apr 16 22:23:06 2019 -0500

    No hints, live output

commit 2942f11
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Apr 16 21:02:28 2019 -0500

    Remove osx, test with stable

commit 85f3865
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Apr 16 18:19:42 2019 -0500

    Remove ospaths, fix tests for Windows

commit 74201bc
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Apr 16 14:00:14 2019 -0500

    No success for missing task

commit 8c2e65e
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Apr 16 13:44:32 2019 -0500

    Fix packageName to name

commit b05d948
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Apr 16 13:29:37 2019 -0500

    Add switch support

commit deecd90
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Apr 16 12:24:01 2019 -0500

    API cleanup, json setCommand fix

commit 1e95fd4
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Apr 16 10:45:12 2019 -0500

    getParams once, hash nimscriptapi, fix loop in setcommand

commit 51d03b3
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Apr 16 07:21:32 2019 -0500

    getPkgDir impl

commit 7d0a40a
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Mon Apr 15 14:24:02 2019 -0500

    Before/after hook info

commit cbb3af3
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Mon Apr 15 13:44:56 2019 -0500

    Remove nims from package dir after exec

commit 0ed53d6
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Sat Apr 13 00:44:26 2019 -0500

    Return bool from hooks

commit ab38b81
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Fri Apr 12 23:20:13 2019 -0500

    Initial version

commit b9ef88b
Merge: 220ebae c8d79fc
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Tue Mar 26 20:16:21 2019 -0500

    Merge remote-tracking branch 'upstream/master' into nocompiler

commit 220ebae
Merge: 3d7227c 119be48
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Wed Dec 12 18:02:10 2018 -0600

    Merge remote-tracking branch 'upstream/master'

commit 3d7227c
Merge: cf7263d 66d79bf
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Wed Oct 17 13:39:51 2018 -0500

    Merge remote-tracking branch 'upstream/master'

commit cf7263d
Merge: 2fc3106 ee4c0ae
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Thu Sep 13 23:03:41 2018 -0500

    Merge remote-tracking branch 'upstream/master'

commit 2fc3106
Merge: e9a8850 c249f9b
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Thu Apr 26 16:27:31 2018 -0500

    Merge remote-tracking branch 'upstream/master'

commit e9a8850
Merge: 7adfd7b 75b7a21
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Thu Mar 8 14:26:46 2018 -0600

    Merge remote-tracking branch 'upstream/master'

commit 7adfd7b
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Mon Jan 15 00:35:55 2018 -0600

    Updated fix for #398

commit de18319
Merge: 93ba4a0 3dae264
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Sun Jan 14 22:01:20 2018 -0600

    Merge remote-tracking branch 'upstream/master'

commit 93ba4a0
Author: Ganesh Viswanathan <dev@genotrance.com>
Date:   Sat Jan 13 19:52:34 2018 -0600

    Fix for #398
@dom96
Copy link
Collaborator

dom96 commented Apr 29, 2019

Merged manually.

@dom96 dom96 closed this Apr 29, 2019
@andreaferretti
Copy link
Contributor

Apparently this PR broke all my packages tests :-D

All my libraries define the tasks nimble tests (a convention suggested in the nimscript docs ), and nimble test as an alias for that, since I usually forget the final s. For instance, in patty nimble file one can find

task tests, "run tests":
  --hints: off
  --linedir: on
  --stacktrace: on
  --linetrace: on
  --debuginfo
  --path: "."
  --run
  setCommand "c", "test.nim"

task test, "run tests":
  setCommand "tests"

After this PR, if I run nimble test on patty I get

andrea$ nimble test
  Executing task test in /Users/andrea/esperimenti/patty/patty.nimble
tests        run tests
test        run tests
   Warning: No tests found!

while running nimble tests gives an error

andrea$ nimble tests
  Executing task tests in /Users/andrea/esperimenti/patty/patty.nimble
../../../lib/pure/parsejson.nim(522) raiseParseErr
Error: unhandled exception: input(1, 153) Error: ] expected [JsonParsingError]

The same happens for all my other libraries. :-(

@genotrance
Copy link
Contributor Author

@andreaferretti - I've made a fix in #642.

@andreaferretti
Copy link
Contributor

@genotrance Thank you, now everything works again :-)

@andreaferretti
Copy link
Contributor

@genotrance Actually, I still find a strange issue in Rosencrantz. Since the tests require a server and a client, I have a small shell script to launch both and stop the server after the tests are done:

set -e
set -u

nimble server
tests/rosencrantz &
PID="$!"
nimble client
kill "$PID"

It seems that the tests pass when launched manually. When I use the script, the tests do not pass (the returned value is not 0) and I get this strange line at the end:

(1, 22) Error: undeclared identifier: 'Users'
     Error: {"success": true, "command": "e", "flags": {"forceBuild": [""]}, "retVal": true}

which is even weirder cosindering that neither the server nor the client code contains the Users identifier anywhere

@andreaferretti
Copy link
Contributor

@genotrance I get random issue while testing again :-(

For instance, on patty I have the following definition

# patty.nimble

task tests, "run tests":
  --hints: off
  --linedir: on
  --stacktrace: on
  --linetrace: on
  --debuginfo
  --path: "."
  --run
  setCommand "c", "test.nim"

task test, "run tests":
setCommand "tests"

If I run nimble tests everything works, but if I run nimble test (no s), which should be an alias, I get

djerzinski:patty andrea$ nimble test
  Executing task test in /Users/andrea/esperimenti/patty/patty.nimble
tests        run tests
test        run tests
   Warning: No tests found!

If I invert the definition of nimble test and nimble tests, both commands fail in the same way. Same thing happens with alea.

This is starting to get brittle. I have automated tests for my libraries, but I just cannot trust them anymore :-(

@dom96
Copy link
Collaborator

dom96 commented May 30, 2019

I noticed something similar for one of my tasks called server, if I renamed it to fooserver it worked, but for some reason server didn't (other tasks in the same file called things like client also worked).

We need to fix this and release a hotfix Nimble before Nim 0.20.0 is out

@genotrance
Copy link
Contributor Author

I'll look at these issues asap.

@genotrance
Copy link
Contributor Author

I've figured out the bug - it is fragile since I'm avoiding projectPath() recently added in Nim devel. I did that so this new nimble could support v0.19.x as well. I'll make it more robust tomorrow.

@dom96
Copy link
Collaborator

dom96 commented May 31, 2019

What are you using instead?

@genotrance
Copy link
Contributor Author

I've created a fix - can you please verify on your end that it fixes your issues?

#659

@andreaferretti
Copy link
Contributor

Sure, but I'm away from computers now, I can check on Monday

@andreaferretti
Copy link
Contributor

@genotrance Works for me, thank you!

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.

4 participants