FIX: Add lapack as build dependency for ipopt 3.13

conda-forge / cyipopt-feedstock

A conda-smithy repository for cyipopt.

BSD 3-Clause "New" or "Revised" License

3 stars 5 forks source link

FIX: Add lapack as build dependency for ipopt 3.13 #21

Closed richardotis closed 4 years ago

richardotis commented 4 years ago

Checklist

[x] Used a fork of the feedstock to propose changes
[x] Bumped the build number (if the version is unchanged)
[x] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[x] Ensured the license file is being packaged.

Fixes #18

Now that ipopt 3.13 is vendoring some dependencies, it appears at least one implicit dependency (lapack) that was getting pulled in by upstream conda-forge recipes is now missing. This fix adds lapack as an explicit build-time dependency, which seems to fix the build errors and allows the test to pass.

conda-forge-linter commented 4 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.

richardotis commented 4 years ago

@conda-forge-admin, please rerender

github-actions[bot] commented 4 years ago

Hi! This is the friendly automated conda-forge-webservice. I tried to rerender for you, but it looks like there was nothing to do.

moorepants commented 4 years ago

Thanks for figuring that out. We can merge this as is, but shouldn't ipopt be using the conda-forge lapack (if it isn't)? Maybe this doesn't matter with lapack, but it does with blas, hopefully the vendored lapack si built with the conda-forge blas.

richardotis commented 4 years ago

I don't have the background to answer that question, but I can confirm that my downstream test suite passes with roughly the same performance characteristics as when using ipopt 3.12.13. I'm not saying that it's comprehensive, but I was getting a few test failures with the other MSVC-based ipopt I was trying to build on Windows, so it is sensitive to numerical precision issues.

moorepants commented 4 years ago

Also, are there no consequences that cyipopt is built with conda forge's lapack and ipopt uses its vendored lapack?

richardotis commented 4 years ago

I'm not sure if cyipopt is actually using lapack here, or if it is just overlinking because it's what pkg-config returns.

moorepants commented 4 years ago

I guess, let's merge and see if issues arise. Can you add the note in the file though?

moorepants commented 4 years ago

LGTM!

richardotis commented 4 years ago

Please merge when ready.