search

conda-forge / casadi-feedstock

A conda-smithy repository for casadi.
BSD 3-Clause "New" or "Revised" License
8 stars 12 forks source link

Update casadi to 3.6.0 and add proxsuite dependency #84

Closed traversaro closed 1 year ago

traversaro commented 1 year ago

Checklist

conda-forge-webservices[bot] commented 1 year 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.

traversaro commented 1 year ago

@conda-forge-admin, please rerender

traversaro commented 1 year ago

PR and issues that were merged and/or solved and so I removed them from the recipe:

traversaro commented 1 year ago

To fix the recipe, I had to submit some other PRs upstream:

Support for proxqp needs a bit more work, so I will leave that for another PR.

traversaro commented 1 year ago

CI seems happy. @conda-forge/casadi do you have any opinion on this?

jgillis commented 1 year ago

Hi @traversaro I'm not familiar with conda builds, but 3.6 may offer extra opportunities: all commercial solvers now come with a WITHMOCKUP option. Also we make source archives now. These avoid a dependency on swig and come with cleaner overload doc in the docstrings

traversaro commented 1 year ago

Hi @traversaro I'm not familiar with conda builds, but 3.6 may offer extra opportunities: all commercial solvers now come with a WITHMOCKUP option. Also we make source archives now. These avoid a dependency on swig and come with cleaner overload doc in the docstrings

Cool, I switched to the source archive in https://github.com/conda-forge/casadi-feedstock/pull/84/commits/dbb68f82990f70c130b4de89be68039ac1531729, so that we have the cleaner overload docs that you mentioned. For the time being I kept the dependency on swig and the geneation of the bindings, as I imagine this is what most users of conda-forge would expect. Anyhow, the swig dependency is just a build dependency so it has no effects on users that install the package via conda/mamba.

Regarding the WITH_MOCKUP_<..> options, what is the advantage in enabling them?

jgillis commented 1 year ago

Cool to see you move so quickly.

dbb68f8 has a null effect, since the extra docstring parts is generated by swig itself (our custom branch, also available in docker ghcr.io/casadi/ci-swig:latest / https://github.com/casadi/casadi/blob/master/misc/Dockerfile.swig ).

-DWITH_KNITRO=ON paired with -DWITH_MOCKUP_KNITRO=ON would allow your users to use nlpsol("solver","knitro",..) without the feedstock having access to KNITRO. It would be picked up at runtime when the user has it installed..

traversaro commented 1 year ago

dbb68f8 has a null effect, since the extra docstring parts is generated by swig itself (our custom branch, also available in docker ghcr.io/casadi/ci-swig:latest / https://github.com/casadi/casadi/blob/master/misc/Dockerfile.swig ).

Ahhh, I see, that make sense, ok let's use your generated code then.

-DWITH_KNITRO=ON paired with -DWITH_MOCKUP_KNITRO=ON would allow your users to use nlpsol("solver","knitro",..) without the feedstock having access to KNITRO. It would be picked up at runtime when the user has it installed..

Wow, that is great! I added those in https://github.com/conda-forge/casadi-feedstock/pull/84/commits/ad5754f20c32696bc3cee315178ad52d2c615b5e, let's see if everything compiles fine.

conda-forge-webservices[bot] commented 1 year 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-webservices[bot] commented 1 year 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.

traversaro commented 1 year ago

dbb68f8 has a null effect, since the extra docstring parts is generated by swig itself (our custom branch, also available in docker ghcr.io/casadi/ci-swig:latest / https://github.com/casadi/casadi/blob/master/misc/Dockerfile.swig ).

Ahhh, I see, that make sense, ok let's use your generated code then.

I tried to use this option, but it seems that SWIG_IMPORT=ON option is not compatible with WITH_JSON=ON, see https://github.com/casadi/casadi/issues/3085 . I do not know if anyone is actually using the WITH_JSON functionality, but given that it has been enabled since 2017 (see https://github.com/conda-forge/casadi-feedstock/commit/32f55e67d80ca4cb51c609d34470ef00484ad4c1), I would prefer not to disable it.

traversaro commented 1 year ago

The build with and is failing with error:

2023-04-07T09:26:50.9906285Z ninja: error: 'external_projects/mockups/knitro/lib/knitro.lib', needed by 'casadi_nlpsol_knitro.dll', missing and no known rule to make it

On all OSs

traversaro commented 1 year ago

dbb68f8 has a null effect, since the extra docstring parts is generated by swig itself (our custom branch, also available in docker ghcr.io/casadi/ci-swig:latest / https://github.com/casadi/casadi/blob/master/misc/Dockerfile.swig ).

Ahhh, I see, that make sense, ok let's use your generated code then.

I tried to use this option, but it seems that SWIG_IMPORT=ON option is not compatible with WITH_JSON=ON, see casadi/casadi#3085 . I do not know if anyone is actually using the WITH_JSON functionality, but given that it has been enabled since 2017 (see 32f55e6), I would prefer not to disable it.

Given https://github.com/casadi/casadi/issues/3085#issuecomment-1500107094 it seems that the json interface is only useful once when haskell bindings were there, but those are not build. Probably we can just disable it, and we can look into re-enabling it if somebody complains.

traversaro commented 1 year ago

The build with and is failing with error:

2023-04-07T09:26:50.9906285Z ninja: error: 'external_projects/mockups/knitro/lib/knitro.lib', needed by 'casadi_nlpsol_knitro.dll', missing and no known rule to make it

On all OSs

I obtain several errors for other commercial solvers:


(casadidev) C:\src\casadi-source-v3.6.0\build>ninja
ninja: error: 'external_projects/mockups/knitro/lib/knitro.lib', needed by 'casadi_nlpsol_knitro.dll', missing and no known rule to make it

(casadidev) C:\src\casadi-source-v3.6.0\build>ccmake .

(casadidev) C:\src\casadi-source-v3.6.0\build>ninja
ninja: error: 'external_projects/mockups/gurobi/lib/gurobi_adaptor.lib', needed by 'casadi_conic_gurobi.dll', missing and no known rule to make it

(casadidev) C:\src\casadi-source-v3.6.0\build>ccmake .

(casadidev) C:\src\casadi-source-v3.6.0\build>ninja
ninja: error: 'external_projects/mockups/snopt/lib/snopt7.lib', needed by 'casadi_nlpsol_snopt.dll', missing and no known rule to make it

(casadidev) C:\src\casadi-source-v3.6.0\build>

As this PR has already quite some changes, probably we can disable the mockup for now and try to enable them in a future PR.

traversaro commented 1 year ago

Ok, the CI jobs seems happy. Any comment on this @conda-forge/casadi ?

github-actions[bot] commented 1 year ago

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

Thus the PR was passing and merged! Have a great day!