jump-dev / ECOS.jl

A Julia interface to the ECOS conic optimization solver
https://github.com/embotech/ecos
Other
41 stars 17 forks source link

Make package compatible with system images #107

Closed omus closed 4 years ago

omus commented 4 years ago

As both deps/build.jl and the compat requirement for ECOS_jll enforce specify that libecos 2.0.5 we should be safe to drop any code to support older versions of libecos. In the future if ECOS.jl can support other versions of libecos this can be enforced by specifying the compat for ECOS_jll appropriately.

Fixes: https://github.com/jump-dev/ECOS.jl/issues/106

codecov[bot] commented 4 years ago

Codecov Report

Merging #107 into master will increase coverage by 0.18%. The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
+ Coverage   83.87%   84.05%   +0.18%     
==========================================
  Files           4        4              
  Lines         465      464       -1     
==========================================
  Hits          390      390              
+ Misses         75       74       -1     
Impacted Files Coverage Δ
src/types.jl 100.00% <ø> (ø)
src/ECOS.jl 96.29% <66.66%> (+3.43%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 39be0c8...f7c55f9. Read the comment docs.

mlubin commented 4 years ago

The README has a section on custom installations: https://github.com/jump-dev/ECOS.jl#custom-installation. No idea if this still works with ECOS_jll. If it does, we still have to be careful about library versions.

blegat commented 4 years ago

We could check the ECOS version and error it is not what we expect

mlubin commented 4 years ago

We could check the ECOS version and error it is not what we expect

This PR deletes code in __init__ that does this.

omus commented 4 years ago

We could check the ECOS version and error it is not what we expect

This PR deletes code in __init__ that does this.

I would say drop support for custom installations. The __init__ check can safely be re-introduced but I removed it as the error message was extremely out of date and still mentioned using Homebrew.jl which as been replaced by the new BinaryProvider/BinaryBuilder setup.

blegat commented 4 years ago

Since artifacts can be overwritten: https://julialang.github.io/Pkg.jl/v1/artifacts/#Overriding-artifact-locations-1 we are not sure about the version used by the jll. In fact, IMO this should be the new recommended way the user should use custom version of solver library. With this approach, we don't need to do anything specific to allow custom version, it's built in Pkg.

omus commented 4 years ago

I've updated the README documentation to let users know custom installations are supported via Pkg's artifact overriding. To ensure the user has a compatible version of libecos installed I've re-introduced the __init__ version check.

omus commented 4 years ago

If it's important we can support libecos versions before 2.0.5 but that will require us to dynamically create the Cpwork struct during the __init__ call.

omus commented 4 years ago

I'd appreciate a release with this included when you get the chance. Thanks!