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

Simpler soft erroring during load #63

Merged
merged 4 commits into from
Mar 25, 2020

Conversation

IanButterworth
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

IanButterworth commented Mar 23, 2020

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
  1. 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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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

@IanButterworth
Copy link
Collaborator Author

Absolutely

@samuelpowell
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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

@IanButterworth
Copy link
Collaborator Author

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

@samuelpowell samuelpowell merged commit c8a176c into samuelpowell:master Mar 25, 2020
@samuelpowell
Copy link
Owner

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

@IanButterworth IanButterworth deleted the simpler_softerroring branch March 25, 2020 14:16
@IanButterworth
Copy link
Collaborator Author

Yeah 👍🏻

@samuelpowell
Copy link
Owner

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

New version registration in progress.

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.

2 participants