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

Fix install name in OSX #32

Merged
merged 3 commits into from
Jul 26, 2017
Merged

Fix install name in OSX #32

merged 3 commits into from
Jul 26, 2017

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Jul 26, 2017

Fixes the install name of the dylib so that the downstream projects will link to libopenblas.dylib instead of libopenblasp-r0.2.20.dylib
In Linux, SONAME is libopenblas.so.0 instead of libopenblasp-r0.2.20.so, so no change needed.

This change means the pinning doesn't have to be restrictive as before (conda-forge/conda-forge.github.io#418) and can be as below

requirements:
   build:
     - openblas 0.2.20
   run:
     - openblas >=0.2.15

One other advantage is that this achieves compatibility with the defaults openblas and therefore a package compiled with conda-forge openblas can use the defaults' openblas at runtime.

cc @ocefpaf

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 26, 2017

   run:
     - openblas >=0.2.15

If this is true we can still build with openblas 0.2.19 as long as the same fixes are applied there, right?

(Just asking. I am +1 to build everything openblas 0.2.20.)

We also need finish the pin build/run PR to pin_slow_way.py to use this.

# Needs to fix the install name of the dylib so that the downstream projects will link
# to libopenblas.dylib instead of libopenblasp-r0.2.20.dylib
# In linux, SONAME is libopenblas.so.0 instead of libopenblasp-r0.2.20.so, so no change needed
install_name_tool -id ${PREFIX}/lib/libopenblas.dylib ${PREFIX}/lib/libopenblas.dylib;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mingwandroid I would like to know your opinion here.

@mingwandroid
Copy link

Sounds perfectly reasonable. All downstreams will need to be rebuilt though to gain the benefit.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 26, 2017 via email

@ocefpaf ocefpaf merged commit d5c564f into conda-forge:master Jul 26, 2017
@isuruf isuruf deleted the osx branch July 26, 2017 12:23
@isuruf
Copy link
Member Author

isuruf commented Jul 26, 2017

Thanks for merging.

If this is true we can still build with openblas 0.2.19 as long as the same fixes are applied there, right?

Yes, but it'll be hard to keep track of which build number of openblas it was built against, so a new version (0.2.20) is better.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 26, 2017

Yes, but it'll be hard to keep track of which build number of openblas it was built against, so a new version (0.2.20) is better.

Agreed. But then the pinning should be:

requirements:
   build:
     - openblas 0.2.20
   run:
     - openblas >=0.2.20

to be on the safe side, right?

@isuruf
Copy link
Member Author

isuruf commented Jul 26, 2017

Advantage of openblas >=0.2.15 is that packages compiled with 0.2.20 can be used with packages compiled with 0.2.19. According to https://abi-laboratory.pro/tracker/timeline/openblas/ no symbols were added or removed from 0.2.15-0.2.20. (Ignore the changed soname in openblas 0.2.20, they have created a debug build). I think that means we have forward compatibility.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 26, 2017

(Ignore the changed soname in openblas 0.2.20, they have created a debug build). I think that means we have forward compatibility.

I am under the impression that the soname will be would be an issue with the 0.2.19 that we have in the channel. If not let's use openblas >=0.2.15.

@isuruf
Copy link
Member Author

isuruf commented Jul 26, 2017

soname is libopenblas.so.0 in linux and libopenblas.dylib in osx with the 0.2.20 package. Even though the sonames are different in 0.2.19, both of these lbraries are available in the 0.2.19 package

@ocefpaf
Copy link
Member

ocefpaf commented Jul 26, 2017

Got it ! Thanks for the explanation @isuruf. I'll start using that pinning scheme. (Although we need to improve the pinning script to do the same.)

@jakirkham
Copy link
Member

Isn't libopenblas.dylib a symlink though? If so, I don't see how the older packages will work.

@isuruf
Copy link
Member Author

isuruf commented Jul 27, 2017

@jakirkham, what I was saying was if you have A compiled with 0.2.19 and B compiled with 0.2.20 you can have A and B with 0.2.19 at runtime.

@isuruf
Copy link
Member Author

isuruf commented Jul 28, 2017

@ocefpaf, you were right. Pinning should be following because abi-laboratory reports don't show the symbols created when compiling with DYNAMIC_ARCH=1 which may have more symbols when new CPU architectures are added.

requirements:
   build:
     - openblas 0.2.20
   run:
     - openblas >=0.2.20

@ocefpaf
Copy link
Member

ocefpaf commented Jul 28, 2017

Cool. I'll update the PRs. Thanks @isuruf!

(But only numpy is using that now. We need to fix the SciPy issue first.)

@jschueller
Copy link
Contributor

@isuruf Does this affect the rpath ? I'm investigating why delocate is not finding openblas dependencies on osx.

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.

6 participants