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.6 and enable fatrop dependency on Linux #115

Closed conda-forge-admin closed 2 months ago

conda-forge-admin commented 2 months ago

Hi! This is the friendly automated conda-forge-webservice.

I've started a version update as instructed in #114.

I'm currently searching for new versions and will update this PR shortly if I find one! Thank you for waiting!

conda-forge-webservices[bot] commented 2 months 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/meta.yaml) and found it was in an excellent condition.

agriyakhetarpal commented 2 months ago

I see that the second patch is failing to apply. CasADi changed the line from friend to inline friend, which is easy to fix.

agriyakhetarpal commented 2 months ago

Also, we had fixed the first patch upstream, so it shouldn't be needed either: https://github.com/casadi/casadi/pull/3759

traversaro commented 2 months ago

Also, we had fixed the first patch upstream, so it shouldn't be needed either: casadi/casadi#3759

Sorry, I am not sure I am following, it seems that https://github.com/conda-forge-admin/casadi-feedstock/blob/conda_forge_admin_114/recipe/patches/0001-Modernize-Python-find_package.patch is not an overlap of https://github.com/casadi/casadi/pull/3759, or I am missing something?

traversaro commented 2 months ago

I see that the second patch is failing to apply. CasADi changed the line from friend to inline friend, which is easy to fix.

For reference, this https://github.com/casadi/casadi/commit/3a30c9ea236226d02d9cbe45e8ac9c59b1a527c6, https://github.com/casadi/casadi/pull/3724 and https://github.com/casadi/casadi/issues/3723 are the related pointers. From what I understand we can probably just drop the patch at all.

traversaro commented 2 months ago

@agriyakhetarpal for the future, if you depend on casadi and you are interested in being an additional mantainer for the feedstock to reduce the bus factor, feel free to add yourself, thanks!

agriyakhetarpal commented 2 months ago

Hi @traversaro, yes, patch 0001 wasn't an overlap of casadi/casadi#3759 – I might not have looked at the entire patch, sorry. The PR fixed the issue where the Python include directory was being searched for by the CasadiGetPythonVersionMajor CMake function, so I think the relevant parts could be restored. But yes, it's easier to just say "Python 3" is the major Python version since Python 2 bindings are not being generated.

I see that the second patch is failing to apply. CasADi changed the line from friend to inline friend, which is easy to fix.

For reference, this casadi/casadi@3a30c9e, casadi/casadi#3724 and casadi/casadi#3723 are the related pointers. From what I understand we can probably just drop the patch at all.

Sure, this makes sense to me!

@agriyakhetarpal for the future, if you depend on casadi and you are interested in being an additional mantainer for the feedstock to reduce the bus factor, feel free to add yourself, thanks!

I'd love to be an additional maintainer – thanks for the offer! I will say that I don't fully understand the CasADi build procedure in depth, but I'll provide a helping hand to you and other maintainers wherever I can. I'll trigger a bot PR for this.

As for the failing Linux builds here, the location of ocp/OCPAbstract.hpp should be fixable enough, just changing to fatrop/ocp/OCPAbstract.hpp should fix it 😄 Edit: and I notice my suggestion is present in https://github.com/NixOS/nixpkgs/blob/3007f981ee958d8e7607a7c5a2de09e634cafc4c/pkgs/by-name/ca/casadi/package.nix#L69-L72 already, so we're good!

traversaro commented 2 months ago

As for the failing Linux builds here, the location of ocp/OCPAbstract.hpp should be fixable enough, just changing to fatrop/ocp/OCPAbstract.hpp should fix it 😄 Edit: and I notice my suggestion is present in https://github.com/NixOS/nixpkgs/blob/3007f981ee958d8e7607a7c5a2de09e634cafc4c/pkgs/by-name/ca/casadi/package.nix#L69-L72 already, so we're good!

Thanks, I proposed a fix upstream in https://github.com/casadi/casadi/pull/3832 .

traversaro commented 2 months ago

@agriyakhetarpal feel free to check the PR, as far as I can see I think it is ready for merge.

github-actions[bot] commented 2 months 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!

agriyakhetarpal commented 2 months ago

Thanks for upstreaming it!