search

conda-forge / suitesparse-feedstock

A conda-smithy repository for suitesparse.
BSD 3-Clause "New" or "Revised" License
1 stars 16 forks source link

patch for setting install_name during install #23

Closed minrk closed 5 years ago

minrk commented 7 years ago

I've submitted this patch upstream. Not sure yet if it will be accepted.

closes #22

conda-forge-linter commented 7 years ago

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.

basnijholt commented 7 years ago

LGTM 👍

@jakirkham @grlee77 what do you think?

grlee77 commented 7 years ago

I am not familiar enough with Makefile syntax to judge if this is correct, so I would defer to those with more expertise. Two quick questions:

1.) In short, this patch only modifies behavior on OS X and causes the dynamic libraries to use e.g. *.4.5.dylib rather than the more specific *.4.5.4.dylib to be used as the install_name?

2.) Do we know that SuiteSparse has ABI compatibility between patch versions?

minrk commented 7 years ago

In short, this patch only modifies behavior on OS X and causes the dynamic libraries to use e.g. .4.5.dylib rather than the more specific .4.5.4.dylib to be used as the install_name?

According to the variables in the Makefiles, it would be *.4.dylib (major only), but otherwise yes.

Do we know that SuiteSparse has ABI compatibility between patch versions?

We know that suitesparse is setting the compatibility version in its dylibs, so at least the libraries themselves claim compatibility at the major version level. I can't speak for the project in general, but a cursory test of a conda package compiled against 4.5.3 updated manually to load 4.5.4 does work.

grlee77 commented 7 years ago

sorry this PR has gone neglected. I think now the build number should be bumped to 202 since the OpenBLAS update bumped it to 201.

@minrk: Is this otherwise good to go? if interested, please add yourself as a maintainer as well

conda-forge-linter commented 7 years ago

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

I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

conda-forge-linter commented 7 years ago

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.

jakirkham commented 7 years ago

According to the ABI report for SuiteSparse it appears that at least 4.5 has been stable across patch release. Before that it was static libraries that are fairly old, which makes it hard to say much about its compatibility during that period.

jakirkham commented 6 years ago

Should we add a file test to ensure that the library name is as expected?

minrk commented 6 years ago

Good idea, I'll add a few calls to otool -L that verify the install name is right.

Aside: it would be cool if conda-build test environments could verify compatibility by running the test env with multiple versions of the built package.

In general, a useful pattern for verifying C libs that their linking config is set up correctly:

  1. conda-build the lib
  2. conda-build a test package that links the lib
  3. install and test the test package against the oldest and newest versions of the lib that its dependency matrix would allow
minrk commented 6 years ago

Sorry for taking so long with the tests. The tests are here now, so we can start getting a suitesparse with correct linking on mac.

conda-forge-linter commented 6 years ago

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

conda-forge-linter commented 6 years ago

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.

minrk commented 6 years ago

This is now also a full conda-build 3 update per @isuruf's request.

minrk commented 6 years ago

Rebased again

conda-forge-linter commented 5 years ago

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.

I do have some suggestions for making it better though...

For recipe:

conda-forge-linter commented 5 years ago

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.

minrk commented 5 years ago

I think this is good to go now (and has been for a year or so). Anything else blocking the merge?

SylvainCorlay commented 5 years ago

:+1:

minrk commented 5 years ago

Conflicts resolved again, build number bumped again.

minrk commented 5 years ago

All builds have succeeded. This should be ready to go again. Not sure why osx shows as still queued, since it's succeeded a while ago.