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

Add support for libraw snapshots #107

Merged
merged 2 commits into from
Apr 12, 2020

Conversation

harshithdwivedi
Copy link
Contributor

@harshithdwivedi harshithdwivedi commented Apr 9, 2020

  • Updates the libraw submodule to the latest version.
  • Bumps the rawpy version to 0.15.0a1

Fixes #106

Signed-off-by: Harshit Dwivedi harshit@pitech.app

@harshithdwivedi harshithdwivedi changed the title feat: update submodules and version Add support for libraw snapshots Apr 9, 2020
@harshithdwivedi
Copy link
Contributor Author

The OSX build failed; as expected. So I'll look into debugging delocate to see if something can be done about it.

Signed-off-by: Harshit Dwivedi <harshit@pitech.app>
@harshithdwivedi
Copy link
Contributor Author

@letmaik Looks like it's trying to copy libjpeg from 2 different places indeed!
I added some logging to the delocation tool and here's what I get:

Required lib is /usr/local/Cellar/little-cms/1.19_1/lib/liblcms.1.0.19.dylib and its lib path is:
/private/var/folders/tg/7qnjtyr57fzd3c32rs2mlm3w0000gp/T/tmplo7oq_09/wheel/rawpy/.dylibs

Required lib is /Users/harshitdwivedi/Desktop/rawpy/external/libs/lib/libjpeg.9.dylib and its lib path is: 
/private/var/folders/tg/7qnjtyr57fzd3c32rs2mlm3w0000gp/T/tmplo7oq_09/wheel/rawpy/.dylibs

Required lib is /usr/local/Cellar/jpeg/9d/lib/libjpeg.9.dylib and its lib path is: 
/private/var/folders/tg/7qnjtyr57fzd3c32rs2mlm3w0000gp/T/tmplo7oq_09/wheel/rawpy/.dylibs

While the error makes sense on my local machine since I have already installed libjpeg with brew, it shouldn't be happening in a CI environment.
A solution could be to not copy the libjpeg from rawpy and instead depend on the libjpeg provided by the OS.
What do you make out of this?

@letmaik
Copy link
Owner

letmaik commented Apr 10, 2020

OK, interesting. Thanks for debugging this! What your log doesn't show is which library is depending on libjpeg. On Linux getting this info is trivial with ldtree. Maybe otool for macOS can do something similar? Either way figuring out the complete dependency chain is vital and I think worth to dump to CI for future analysis.

@harshithdwivedi
Copy link
Contributor Author

I tried otooling all the dylibs that rawpy ships with and it seems like libjpeg is a dpendency of both libraw and libjasper.

Libraw picks the libjpeg shipped with rawpy whereas libjasper picks the one in /usr/local/opt

(base) ➜  Desktop otool -L /Users/harshitdwivedi/Desktop/rawpy/external/LibRaw/cmake_build/libraw_r.19.0.0.dylib 
/Users/harshitdwivedi/Desktop/rawpy/external/LibRaw/cmake_build/libraw_r.19.0.0.dylib:
	/Users/harshitdwivedi/Desktop/rawpy/external/LibRaw/cmake_build/libraw_r.19.dylib (compatibility version 19.0.0, current version 19.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.0.0)
	/usr/local/opt/little-cms/lib/liblcms.1.dylib (compatibility version 2.0.0, current version 2.19.0)
	/usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.11)
	/Users/harshitdwivedi/Desktop/rawpy/external/libs/lib/libjpeg.9.dylib (compatibility version 13.0.0, current version 13.0.0)
	/Users/harshitdwivedi/Desktop/rawpy/external/libs/lib/libjasper.4.dylib (compatibility version 4.0.0, current version 5.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 800.7.0)

(base) ➜  Desktop otool -L /Users/harshitdwivedi/Desktop/rawpy/external/libs/lib/libjasper.5.0.0.dylib 
/Users/harshitdwivedi/Desktop/rawpy/external/libs/lib/libjasper.5.0.0.dylib:
	/Users/harshitdwivedi/Desktop/rawpy/external/libs/lib/libjasper.4.dylib (compatibility version 4.0.0, current version 5.0.0)
	/usr/local/opt/jpeg/lib/libjpeg.9.dylib (compatibility version 14.0.0, current version 14.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.0.0)

(base) ➜  Desktop otool -L /Users/harshitdwivedi/Desktop/rawpy/external/libs/lib/liblcms2.2.dylib 
/Users/harshitdwivedi/Desktop/rawpy/external/libs/lib/liblcms2.2.dylib:
	/Users/harshitdwivedi/Desktop/rawpy/external/libs/lib/liblcms2.2.dylib (compatibility version 3.0.0, current version 3.8.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.0.0)

@letmaik
Copy link
Owner

letmaik commented Apr 10, 2020

Good job, I think I know what's going on. Basically, we build libjpeg ourselves but then don't tell cmake about it when building libjasper. I moved the export CMAKE_PREFIX_PATH=$LIB_INSTALL_PREFIX line further up which should fix it. I'm currently running a build: https://github.com/letmaik/rawpy/runs/577008495?check_suite_focus=true. Once that succeeds I'll push to master and merge into prerelease to unblock you.

@letmaik
Copy link
Owner

letmaik commented Apr 10, 2020

Alright, all green again :) Just merge in the current prerelease branch and you should be good to go.

@harshithdwivedi
Copy link
Contributor Author

Sounds like a plan!

@harshithdwivedi
Copy link
Contributor Author

harshithdwivedi commented Apr 10, 2020

Oh, and while we're at it; can we build and package version 14.0.0 of libjpeg instead of 13.0.0?
I was trying this on my system and apparently on packaging the app with PyInstaller, there were some conflicts with PIL and libjpeg version.

Not a big deal, but if rawpy works well with version 14, then no reason to have it ship version 13, right?

@letmaik
Copy link
Owner

letmaik commented Apr 10, 2020

Sure, I don't mind. I'm assuming v14 is equivalent to v9d at http://ijg.org/files/? If so, can you open a separate PR that bumps the version?

@harshithdwivedi
Copy link
Contributor Author

harshithdwivedi commented Apr 10, 2020

Yep, certainly.
Can you merge this one? I'll open one post that

@letmaik
Copy link
Owner

letmaik commented Apr 10, 2020

Similarly to Windows we may want to think about building static libraries only for the dependencies on macOS. This would avoid such conflicts.

@harshithdwivedi
Copy link
Contributor Author

Fair enough, that makes sense.

@harshithdwivedi
Copy link
Contributor Author

@letmaik I think this is good for merge.

@letmaik letmaik merged commit c8d3dbd into letmaik:prerelease Apr 12, 2020
@harshithdwivedi harshithdwivedi deleted the prerelease_wip branch April 12, 2020 08:33
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