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

Compiler: store and restore debug location when going to main #8234

Merged
merged 1 commit into from
Sep 25, 2019

Conversation

asterite
Copy link
Member

Fixes #8233
Fixes #7060
Fixes #6920

(which are all just the same bug, but this way it looks like this is a killer PR! 😛)

@bcardiff I'm putting this for 0.31.1 because even though it's not a bug introduced there, it's something that will likely fix a lot of the existing issues with debug info.

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:debugger labels Sep 25, 2019
@asterite asterite added this to the 0.31.1 milestone Sep 25, 2019
@paulcsmith
Copy link
Contributor

Woo hoo! This is awesome. I’ve run into this a couple times

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

sure! Lets make it to 0.31.1

@asterite asterite merged commit 7ebfd3d into master Sep 25, 2019
@asterite asterite deleted the bug/main-debug-location branch September 25, 2019 21:17
@paulcsmith
Copy link
Contributor

paulcsmith commented Sep 25, 2019

@bcardiff When do you think 0.31.1 could be released. Right now we're blocked on releasing a new version of Lucky because we can't shutdown HTTP servers/chromedriver in specs (due to the specs being run in at_exit now). We tried monkey patching Spec but it raises this compile time error so we have to run specs with --no-debug to get them to run :S

Long story short, if this was released we'd be able to get a new version of Lucky out with support for the newest Crystal. Don't want to pressure anything, just explain the situation and see if that may affect the release cycle. Or if you have some ideas for helping us that'd be great too

Here is what we're doing now that requires --no-debug andI believe would be fixed by this PR https://github.com/luckyframework/lucky_flow/pull/60/files#diff-906deee07fd614ba421ecd7a4a66ffd1R6

cc @jwoertink

@jwoertink
Copy link
Contributor

I can try to run this against master and see how that goes.

@asterite
Copy link
Member Author

@paulcsmith What do we need from Spec's public API to be able to fix the problem? I'm thinking maybe Spec.after(:suite) or something like that could solve it. I'm not sure whether the problem is that at_exit in your code runs before or after Spec.

@asterite
Copy link
Member Author

Oh, I just see #8235

@jwoertink
Copy link
Contributor

Ok, so I ran 2 different repos that had this issue against master, and both of them give me the same response:

[15:57PM] selenium-webdriver-crystal (master)$ /Users/jeremywoertink/Development/crystal/lang/.build/crystal run test/session_test.cr
Showing last frame. Use --error-trace for full trace.

In test/test_helper.cr:2:1

 2 | require "minitest/autorun"
     ^
Error: can't find file 'minitest/autorun'

If you're trying to require a shard:
- Did you remember to run `shards install`?
- Did you make sure you're running the compiler in the same directory as your shard.yml?
[15:58PM] lucky_flow (jaw/v0.6.0)$ /Users/jeremywoertink/Development/crystal/lang/.build/crystal spec/
Showing last frame. Use --error-trace for full trace.

In src/lucky_flow.cr:1:1

 1 | require "selenium"
     ^
Error: can't find file 'selenium'

If you're trying to require a shard:
- Did you remember to run `shards install`?
- Did you make sure you're running the compiler in the same directory as your shard.yml?

But both of these throw the module validation error with 0.31.0, so progress! 😄

@paulcsmith
Copy link
Contributor

@asterite Yes the after_suite method would fix this as well and would be the preferred solution since we wouldn't need to monkey patch Spec.

@jwoertink sometimes using Crystal master doesn't get the paths quite right when run like that so I'm guessing there is something up there but I don't know the fix. I think you can mv the built binary to /usr/local/bin/crystal-master or something and then run crystal-master spec

@jwoertink
Copy link
Contributor

@paulcsmith I tried moving the binary to /usr/local/bin, but I get the same error.

[16:03PM] lucky_flow (jaw/v0.6.0)$ crystal_dev spec/
Showing last frame. Use --error-trace for full trace.

In src/lucky_flow.cr:1:1

 1 | require "selenium"
     ^
Error: can't find file 'selenium'

I ran shards install. Maybe it's cache or something? 🤷‍♂

@asterite
Copy link
Member Author

The way to do it is to invoke bin/crystal located in the compiler's source code that you have in your machine. For example:

  • I have Crystal in /Users/asterite/Projects/crystal
  • I run: /Users/asterite/Projects/crystal/bin/crystal program.cr

That bin/crystal wrapper will set CRYSTAL_PATH to use the src from the compiler's source code in that directory, and then lib.

To simplify things I have an alias ccrystal that points to /Users/asterite/Projects/crystal/bin/crystal so to try things with the new compiler I:

  • run make crystal in the compiler's directory
  • use ccrystal to use that new compiler

ccrystal is also useful to test the standard library in master.

Let me know if using any of those methods works for you guys with master. Fingers crossed 🤞

@bcardiff
Copy link
Member

@paulcsmith there are two outstanding regressions / bugs I would like to try fix before 0.31.1 #8230 and #8231 . If things go smooth we should be ready for a release tomorrow or on Monday.

@paulcsmith
Copy link
Contributor

paulcsmith commented Sep 26, 2019 via email

bcardiff pushed a commit that referenced this pull request Sep 27, 2019
Compiler: store and restore debug location when going to main
icy-arctic-fox added a commit to icy-arctic-fox/spectator that referenced this pull request Feb 10, 2021
icy-arctic-fox added a commit to icy-arctic-fox/spectator that referenced this pull request Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:debugger
Projects
None yet
4 participants