Conversation
proc.e is now a union type and the only way to assign values to it is by converting the raw data to a fixed size byte array.
|
Hey! Awesome! If you don't mind at least making sure it tests on 1.13 I would greatly appreciate it. It's in the I can work on 1.12 and getting go modules working after this patch is ready. Thanks for contributing! |
|
oh, to clarify, I think dropping 1.10, and 1.11 support is probably worth it if it causes you to write unnecessary workarounds or other complexity. @mitchellh I think I'm making a safe judgment call, but please shout out if you disagree. |
|
@erikh CI should be working now |
erikh
left a comment
erikh
left a comment
There was a problem hiding this comment.
Looks great! However, two tests are being ignored in the suite, which I think should be resolved or at least discussed before I can comfortably merge this.
| @@ -14,11 +14,11 @@ lint: | |||
| sh golint.sh | |||
|
|
|||
| megacheck: | |||
There was a problem hiding this comment.
can we rename this task to 'staticcheck'? also, nice catch, thanks!
| filename string | ||
| mrb *Mrb | ||
| captureErrors bool | ||
| // captureErrors bool |
There was a problem hiding this comment.
please remove this line if it's not being used
|
|
||
| mrb.Close() | ||
|
|
||
| /* |
There was a problem hiding this comment.
why is this test commented out?
There was a problem hiding this comment.
This causes use after free in sanitizer enabled env and it shouldn't be tested with go test
There was a problem hiding this comment.
I think the major point of this test is to run that block that's commented out; is there any way to re-enable the test? This feels like a change in API.
There was a problem hiding this comment.
I don't care how you feel.
You should never touch freed memory.
| mrb.Close() | ||
|
|
||
| // TODO(take-cheeze): Test below | ||
| return |
There was a problem hiding this comment.
let's make sure we pass all tests before merge.
There was a problem hiding this comment.
I need to fix mruby itself to make this work.
There was a problem hiding this comment.
I need this functionality for my own projects that depend on go-mruby. What changed between 1.2 and 2.1 that caused this to break?
There was a problem hiding this comment.
Totally safe. I think generally N, and N-1 are the versions you want to support which gives you 1 year of backwards support (Go is on 6mo cycles). Go 1.13 in particular was a rough version for a lot of projects to upgrade to due to major changes in Go modules behavior (we struggled at HashiCorp). This is awesome though. Thanks @take-cheeze and @erikh! |
| enable_debug | ||
| end | ||
|
|
||
| gem core: 'mruby-error' |
There was a problem hiding this comment.
I didn't notice this before, and I'm sorry about this. We can't depend on external gems here, I hope you realize. The whole point of having this environment this way is that the user can customize it.
This breaks nearly everything I depend on in this library. Is there any way we can bring in the changes that mruby-error provides directly? We had healthy gc arena code in the codebase before. was it removed?
4 participants
mrb_protectfunction) instead for exception handling.