samuelpowell / Spinnaker.jl

A Julia interface to the FLIR/PointGrey Spinnaker SDK
MIT License
15 stars 7 forks source link

Simpler soft erroring during load #63

Closed IanButterworth closed 4 years ago

IanButterworth commented 4 years ago

I've been seeing odd loading instabilities that might be due to the way the soft erroring was designed.

This is a bit simpler, and my problems went away when I did it.. Although, I'm not certain that this specifically was the fix.

samuelpowell commented 4 years ago

What kind of errors?

Another thought - why don't we just look the library up at init time and do away with the build file altogether? It doesn't actually do much and it makes package stateful which is frowned upon in the new world.

IanButterworth commented 4 years ago

why don't we just look the library up at init time

Yeah.. why not.. It's not like there's huge overhead in finding the lib.

I was seeing situations where the deps.jl file is created successfully, but the precompiled package was claiming it wasn't. The state of __built__ seemed to be unstable with precompilation. That is a guess though, as I didn't properly round-in on the cause.

samuelpowell commented 4 years ago

I think the only reason that it was like this in the beginning was to ensure that the library was present at build time. Once that was circumvented there was no need for there to be any build step.

So we check for and attempt to load the library during init. If it doesn't load the reference to the library remains C_NULL and we print a little warning.

Do you know if ccall will handle this gracefully if a user tries to abuse it, or do we need to guard?

IanButterworth commented 4 years ago

Refactored to just check dlopen during init. Works nicely in the case where the lib is present.

julia> @time using Spinnaker
  0.483671 seconds (881.89 k allocations: 46.705 MiB, 2.20% gc time)

Unfortunately though, it seems we might have a problem on 1.4.0 (above was 1.3.1)

julia> using Spinnaker
ERROR: InitError: could not load library "/usr/local/lib/libSpinnaker_C.dylib"
dlopen(/usr/local/lib/libSpinnaker_C.dylib, 1): Library not loaded: libSpinnaker.dylib.1.27.0.48
  Referenced from: /usr/local/lib/libSpinnaker_C.dylib.1.27.0.48
  Reason: image not found
IanButterworth commented 4 years ago

The 1.4.0 bug seems to be two things..

  1. Spinnaker points libSpinnaker to the wrong path for libusb (not clear to me why this didn't error on 1.3.1) Fixed with:
    sudo install_name_tool -change /opt/local/lib/libusb-1.0.0.dylib /usr/local/lib/libusb-1.0.0.dylib /usr/local/lib/libSpinnaker.dylib
  2. There is a notarization bug in the MacOS official binary for 1.4.0 that results in ibraries that are sitting in /usr/local/lib to no longer be "special" Temporary fix:
    sudo install_name_tool -add_rpath @loader_path/ /usr/local/lib/libSpinnaker.dylib
    sudo install_name_tool -add_rpath @loader_path/ /usr/local/lib/libSpinVideo.dylib
    sudo install_name_tool -change libSpinnaker.dylib.1.27.0.48 @rpath/libSpinnaker.dylib.1.27.0.48 /usr/local/lib/libSpinnaker_C.dylib
    sudo install_name_tool -change libSpinnaker.dylib.1.27.0.48 @rpath/libSpinnaker.dylib.1.27.0.48 /usr/local/lib/libSpinVideo_C.dylib
    sudo install_name_tool -change libSpinnaker.dylib.1.27.0.48 @rpath/libSpinnaker.dylib.1.27.0.48 /usr/local/lib/libSpinVideo.dylib

Thanks to @staticfloat for figuring all this out!

samuelpowell commented 4 years ago

Okay so this still has a build step or searching for the libraries, is that required? What does it do?

Are the above fixes sufficient to make this work on MacOS?

IanButterworth commented 4 years ago

Apologies, the revert to the build method was an error during debugging. I've pushed what I've settled on now. It should give informative errors, and pass on unsupported systems, and those without the SDK present.

Are the above fixes sufficient to make this work on MacOS?

I'm 99% sure.. I haven't reverted my system to a clean install and re-tested

samuelpowell commented 4 years ago

I can do this on 1.3 if you can hold on till tomorrow?

IanButterworth commented 4 years ago

Absolutely

samuelpowell commented 4 years ago

Works fine on virgin environment, 1.3.

It would probably be better if we just have const global ref to the library, and we assign at init, rather than this convoluted baking in of the path at recompilation. But I appreciate it is more work and not critical.

I will update the README, are the two fixes above still required, and only on MacOS?

IanButterworth commented 4 years ago

That doesn’t work. The path to the library is to be a proper const. I tried that initially and ccall complained. There’s a few issues out there about it

samuelpowell commented 4 years ago

How bizarre. and how about the fixes above, are they required on MacOS only on 1.4 only?

IanButterworth commented 4 years ago

Yes but likely even more specific to the macOS 1.4 notarized official binary.

samuelpowell commented 4 years ago

Hmm that was an accident but it's good to go isn't it?

IanButterworth commented 4 years ago

Yeah 👍🏻

samuelpowell commented 4 years ago

Okay good. I got a bit trigger happy on the command line.

New version registration in progress.