JoshCheek / seeing_is_believing

Displays the results of every line of code in your file
1.31k stars 54 forks source link

Relaxing the FFI dependency version constraint #161

Closed janko closed 3 years ago

janko commented 3 years ago

On Mac OS Big Sur (Apple Silicon) I get the following error when attempting to install FFI 1.11.3 (the latest seeing_is_believing currently allows):

FFI install error ``` ERROR: Error installing seeing_is_believing: ERROR: Failed to build gem native extension. current directory: /Users/janko/.rbenv/versions/3.0.1/lib/ruby/gems/3.0.0/gems/ffi-1.11.3/ext/ffi_c /Users/janko/.rbenv/versions/3.0.1/bin/ruby -I /Users/janko/.rbenv/versions/3.0.1/lib/ruby/3.0.0 -r ./siteconf20210519-70338-hgcy0j.rb extconf.rb checking for ffi_call() in -lffi... yes checking for ffi_closure_alloc()... yes checking for shlwapi.h... no checking for rb_thread_call_without_gvl()... yes checking for ruby_native_thread_p()... yes checking for ruby_thread_has_gvl_p()... yes checking for ffi_prep_cif_var()... yes checking for ffi_raw_call()... yes checking for ffi_prep_raw_closure()... yes creating extconf.h creating Makefile current directory: /Users/janko/.rbenv/versions/3.0.1/lib/ruby/gems/3.0.0/gems/ffi-1.11.3/ext/ffi_c make DESTDIR\= clean current directory: /Users/janko/.rbenv/versions/3.0.1/lib/ruby/gems/3.0.0/gems/ffi-1.11.3/ext/ffi_c make DESTDIR\= compiling AbstractMemory.c compiling ArrayType.c compiling Buffer.c compiling Call.c compiling ClosurePool.c compiling DynamicLibrary.c compiling Function.c Function.c:867:17: error: implicit declaration of function 'ffi_prep_closure' is invalid in C99 [-Werror,-Wimplicit-function-declaration] ffiStatus = ffi_prep_closure(code, &fnInfo->ffi_cif, callback_invoke, closure); ^ 1 error generated. make: *** [Function.o] Error 1 make failed, exit code 2 Gem files will remain installed in /Users/janko/.rbenv/versions/3.0.1/lib/ruby/gems/3.0.0/gems/ffi-1.11.3 for inspection. Results logged to /Users/janko/.rbenv/versions/3.0.1/lib/ruby/gems/3.0.0/extensions/arm64-darwin-20/3.0.0/ffi-1.11.3/gem_make.out ```

Installing FFI 1.15.0 (current latest version) works without problem. I've also installed seeing_is_believing with that version, and it works well too.

I think it would be worthwhile to relax the FFI version constraint to allow for major versions.

JoshCheek commented 3 years ago

Hmm, I've been getting warnings that make it pretty much unusable for me:

image

If I do this:

image

Then it works for me on Apple Silicon:

image

I opened an issue with them, but they've not responded: https://github.com/enkessler/childprocess/issues/176

🤔 IDK, maybe disabling its logger isn't the worst solution in the world? Probably better than SiB being broken on the M1.

janko commented 3 years ago

Yeah, I had to disable their logger as well to avoid the warning you mentioned. Maybe it's worth disabling it on M1.

JoshCheek commented 3 years ago

Any chance you've got a linux machine and would be willing to pair? I removed the gem and used Kernel#spawn, and it works on my M1, but fails, seemingly randomly, on CI's Ubuntu 20: https://github.com/JoshCheek/seeing_is_believing/actions/runs/867864608

Not sure how to debug it without accessing such a machine.

janko commented 3 years ago

No unfortunately, I have only Macs.

I appreciate the willingness to make it work, let me know if I can help in any other way.

JoshCheek commented 3 years ago

Well, probably going to switch it back to ChildProcess. I was chatting about this with @zenspider and he got to looking @ the ChildProcess code and made a PR that should hopefully fix it: https://github.com/enkessler/childprocess/pull/177

JoshCheek commented 3 years ago

Sorry for being slow. I'm on 2 projects @ work rn & there's also life, so even though I recognize that this is important, I've been tuning out pretty hard @ EOD.

Went to do it today, but sadly there is a patchlevel difference between the reported host_cpu between Ruby 2.7.2 and 2.7.3, which causes the new version of childprocess to not work on the M1 for Ruby < 2.7.3

$ uname -ra
Darwin Joshs-MacBook-Air.local 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:06:51 PST 2021; root:xnu-7195.81.3~1/RELEASE_ARM64_T8101 arm64

$ chruby-exec 2.7.2 -- ruby -e 'puts RbConfig::CONFIG["host_cpu"]'
arm

$ chruby-exec 2.7.3 -- ruby -e 'puts RbConfig::CONFIG["host_cpu"]'
arm64
janko commented 3 years ago

Thank you, I appreciate the fix! As a fellow open source maintainer, I completely understand the delay 😉 Especially considering that this fix spanned through additional dependencies 😮

For me the above issue on older rubies doesn't matter, as I only needed it for Ruby 3.0+ for now.

JoshCheek commented 3 years ago

thx for understanding ❤️

I'm super torn rn. I've got it working on Linux and Mac (CI), but it fails on Windows, and it looks like it fails pretty hard.

🤔 As I think about it now, I feel like someone left me an issue once with an insight that might be relevant here. I should glance through the issues and see what it was.

I spent most of the day on it, and I kinda want to just release it, but I hate to throw the Windows people under the bus. But man, process management across OSes and ruby versions is not where I want to spend my time >.< Really I should be updating it to understand newer Ruby syntaxes and maybe improving the editor integrations. Anyway, I'm going to have to major version bump it b/c the dependency on FFI went from 3 to 4. 🤔 And since I'm doing that, I should update the parser dependency across its major version, too. I think that'll go smoothly, though, I already read through the changelog and their underlying issues.

I'll try to get something out this week, if Windows becomes unusable, then I guess I'll have to request that the people who notice it help me out, b/c I just don't know how to effectively dig into it.

JoshCheek commented 3 years ago

Oh, also relaxed the dependencies, like the original topic requests: https://github.com/JoshCheek/seeing_is_believing/blob/cea723c42d514475d4b509bfdcb04b1178116e30/seeing_is_believing.gemspec#L20-L22

The minor version can now increase, instead of just the patchlevel. I assume this is the right way to do it, but if you have thoughts on how to do that better, I'm open to suggestions.