Bump ECOS_jll

[odow]

Closes #116

[codecov[bot]]

Codecov Report

Merging #117 (3450871) into master (4094543) will decrease coverage by 16.19%. The diff coverage is 23.78%.

:exclamation: Current head 3450871 differs from pull request most recent head 9c84114. Consider uploading reports for the commit 9c84114 to get more accurate results

@@             Coverage Diff             @@
##           master     #117       +/-   ##
===========================================
- Coverage   85.86%   69.66%   -16.20%     
===========================================
  Files           4        5        +1     
  Lines         481      633      +152     
===========================================
+ Hits          413      441       +28     
- Misses         68      192      +124     

Impacted Files	Coverage Δ
src/gen/libecos_api.jl	4.68% <4.68%> (ø)
src/ECOS.jl	90.00% <84.21%> (-6.43%)	:arrow_down:
src/MOI_wrapper.jl	78.59% <100.00%> (+1.61%)	:arrow_up:
src/MPB_wrapper.jl	94.47% <100.00%> (+0.10%)	:arrow_up:
src/gen/libecos_common.jl	100.00% <100.00%> (ø)

---

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 4094543...9c84114. Read the comment docs.

[odow]

Hmm weird error on Windows only.

Test the direct interface: Error During Test at D:\a\ECOS.jl\ECOS.jl\test\runtests.jl:14
  Got exception outside of a @test
  LoadError: ECOS failed to construct problem.
  Stacktrace:
   [1] error(::String) at .\error.jl:33
   [2] setup(::Int64, ::Int64, ::Int64, ::Int64, ::Int64, ::Array{Int64,1}, ::Int64, ::ECOS.ECOSMatrix, ::ECOS.ECOSMatrix, ::Array{Float64,1}, ::Array{Float64,1}, ::Array{Float64,1}; kwargs::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at D:\a\ECOS.jl\ECOS.jl\src\ECOS.jl:121
   [3] setup at D:\a\ECOS.jl\ECOS.jl\src\ECOS.jl:106 [inlined]
   [4] test1() at D:\a\ECOS.jl\ECOS.jl\test\direct.jl:49
   [5] top-level scope at D:\a\ECOS.jl\ECOS.jl\test\direct.jl:58
   [6] include(::String) at .\client.jl:457
   [7] top-level scope at D:\a\ECOS.jl\ECOS.jl\test\runtests.jl:15
   [8] top-level scope at C:\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.5\Test\src\Test.jl:1119
   [9] top-level scope at D:\a\ECOS.jl\ECOS.jl\test\runtests.jl:15
   [10] include(::String) at .\client.jl:457
   [11] top-level scope at none:6
   [12] eval(::Module, ::Any) at .\boot.jl:347
   [13] exec_options(::Base.JLOptions) at .\client.jl:272
   [14] _start() at .\client.jl:506
  in expression starting at D:\a\ECOS.jl\ECOS.jl\test\direct.jl:58

[odow]

Anyone have ideas (or a windows machine to test on)? I'm guessing we need to set some flag when compiling on windows, but I couldn't see any obvious culprits: https://github.com/embotech/ecos/compare/v2.0.5...v2.0.8.

[metab0t]

@odow I spent an afternoon on this issue and I think this change is the root cause:

https://github.com/embotech/ecos/blob/fcdbcc8dd221b8b2a556b2d90f52e47a749c9695/external/SuiteSparse_config/SuiteSparse_config.h#L54

https://github.com/embotech/ecos/blob/6f55a86080dac75b5df34ce1135cafda1c2053c0/external/SuiteSparse_config/SuiteSparse_config.h#L53

In version 2.0.5, MinGW does not define _MSC_VER so idxint is defined as long the same as other platforms. In current version, idxint is defined as __int64 rather than long on Windows (sizeof(long)=4 but sizeof(__int64)=8 on Windows), but the generated Julia binding still assumes idxint = Clong and caused error.

We can patch this line when building ECOS_jll. I am not familiar with BinaryBuilder so I haven't given it a try.

[odow]

Thanks for looking into this!

It might be sufficient to just change this line: https://github.com/jump-dev/ECOS.jl/blob/34508710f029413b7409f8bb4dc52b5baaae5a56/src/gen/libecos_common.jl#L7 assuming the library is built correctly for Windows.

Something like

const idxint = @static Sys.iswindows() ? Int64 : Clong  # Or something

Isn't Clong just an alias for Int64 though?

julia> Clong
Int64

Or is it different on Windows?

In which case maybe it's just

const idxint = Int64

[metab0t]

Clong is an alias of Int32 on Windows.

This is fine in my opinion because it is identical to the logic in SuiteSparse_config.h.

const idxint = @static Sys.iswindows() ? Int64 : Clong

[odow]

Hmm. I guess CI won't run until I fix the conflicts. If you're interested and have time, please feel free to copy-paste any of the changes I made into a new PR!

[metab0t]

I will try to update the C-API generation script to use the newest Clang.jl when I have more time. Clang.jl has overhauled its generation interface recently.

[odow]

Closing in favor of #128