Add coincbc #11

BastianZim · 2021-10-30T17:59:55Z

Checklist

Used a personal fork of the feedstock to propose changes
Bumped the build number (if the version is unchanged)
Reset the build number to 0 (if the version changed)
Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
Ensured the license file is being packaged.

Based on conda-forge/conda-forge-pinning-feedstock#2080 (comment) this adds the old coincbc package as an additional output of this one. Thus from now on, the coin-or-cbc package will be actually installed if coincbc is installed.

Note to the old feedstock is in here: conda-forge/coincbc-feedstock#37

conda-forge-linter · 2021-10-30T17:59:59Z

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:

Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

conda-forge-linter · 2021-10-30T18:03:58Z

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:

The home item is expected in the about section.
The license item is expected in the about section.
The summary item is expected in the about section.

For recipe:

It looks like the 'coincbc' output doesn't have any tests.

conda-forge-linter · 2021-10-30T18:07:08Z

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.

BastianZim · 2021-10-30T18:08:19Z

@conda-forge-admin, please rerender

…nda-forge-pinning 2021.10.30.17.45.23

BastianZim · 2021-10-30T18:58:39Z

Thanks that fixed it @isuruf! Quick other question: Do you know where the new errors could come from, as I'm not sure if they're caused by this PR? Otherwise, I'd leave it to the maintainers.

BastianZim · 2021-10-30T18:59:16Z

@conda-forge-admin, please rerender

github-actions · 2021-10-30T19:00:36Z

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

BastianZim · 2021-11-01T07:50:22Z

@wolfv Do you see a reason here? I'm not really even sure if this is related to my PR and not something underlying.

wolfv · 2021-11-02T07:50:53Z

Hm, I just checked locally and I see the same issue. It looks like these symbols are in libClp.dylib but not in libOsiClp.dylib (which does link libClp though).

If I manually add -lClp this links fine.

@tkralphs should it be enough to link with -lOsiClp?

wolfv · 2021-11-02T07:51:19Z

This line succeeds:

(/Users/wolfvollprecht/micromamba/conda-bld/coin-or-cbc_1635837522119/_build_env) ➜  src arm64-apple-darwin20.0.0-clang++ -dynamiclib -single_module  -o .libs/libCbc.3.10.5.dylib .libs/libCbc.3.10.5.dylib-master.o  -L/Users/wolfvollprecht/micromamba/conda-bld/coin-or-cbc_1635837522119/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold/lib -lpthread -lreadline -lncurses 
-lClp -lOsiClp -lCgl -lOsi -lCoinUtils  -Wl,-pie -Wl,-headerpad_max_install_names -Wl,-dead_strip_dylibs 
-Wl,-rpath -Wl,/Users/wolfvollprecht/micromamba/conda-bld/coin-or-cbc_1635837522119/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold/lib -install_name  /Users/wolfvollprecht/micromamba/conda-bld/coin-or-cbc_1635837522119/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold/lib/libCbc.3.dylib -compatibility_version 14 -current_version 14.5

BastianZim · 2021-11-02T07:54:05Z

Thanks for the investigation. By the way, any idea why this is failing now? Everything worked during the last rebuild so I’m thinking this might be something in the conda-forge infrastructure.

wolfv · 2021-11-02T08:07:35Z

hmm, well, for one thing the new mambabuild chooses the higher build numbers more consistently I think. Besides that there is nothing that I know of but there might have been a tiny change in the default linker flags that leads to this ...

isuruf · 2021-11-02T15:17:53Z

By the way, any idea why this is failing now? Everything worked during the last rebuild so I’m thinking this might be something in the conda-forge infrastructure.

Well, you were the one who merged conda-forge/coin-or-clp-feedstock#6

BastianZim · 2021-11-02T15:40:45Z

Ahh yes completely forgot that, thanks for the pointer. 😄

tkralphs · 2021-11-02T15:45:57Z

@tkralphs should it be enough to link with -lOsiClp?

I guess it's clear by now, but Cbc has a direct dependency on libClp, not just indirectly through libOsiClp. I haven't looked that closely, but I assume that is the issue?

tkralphs · 2021-11-03T13:06:59Z

It's also a bit of mystery why when I build locally, I do actually get -lClp when building this recipe.

BastianZim · 2021-11-03T13:09:32Z

I have created conda-forge/coin-or-clp-feedstock#9 and conda-forge/coin-or-clp-feedstock#10 to apply the requested changes but I do not know enough about the package to comment for or against the patches.
Just let me know what the outcome is and I'll append the PRs.

BastianZim · 2021-11-03T13:10:45Z

I still don't really know what's going on because it seems to me that Clp needs to be listed as a dependency in the recipe, but it is not currently listed in the production recipe and that seems to work.

Might be a game of whack-a-mole but how about adding this and seeing what happens?

tkralphs · 2021-11-03T14:28:25Z

Might be a game of whack-a-mole but how about adding this and seeing what happens?

I guess it doesn't hurt to try, but we're missing something. We're only seeing a failure on OS X, which is weird. That actually explains why my local build is passing because I built on Linux (duh!). Digging a little deeper, Cbc's configure script does find the Clp package (see here) but then -lClp doesn't make it to the link line for some reason here. That's weird. The error comes when linking libCbc because I guess linker is checking for undefined symbols at library link time. Contrast that to Linux, where the process goes exactly the same way all the way through, including the missing -lClp on the link line, but there are no complaints about missing symbols (see here). I guess that is because we are not using --no-undefined on Linux. That is done by default in our build process, but it looks like all the flags are over-written by conda. Seems like something that should be changed. In the end, the build succeeds on Linux anyway I guess because Clp is a dependency of OsiClp.

So bottom line is that the same thing happens on Linux, it just doesn't cause a failure in the end. So I guess I can dig into my Linux build and try to fix it there. Perhaps adding Clp as a direct dependency will fix things, but -lClp should be added to the link line in the configure step either way. After all, -lCoinUtils is there and that is not listed as a dependency either.

wolfv · 2021-11-03T14:36:59Z

We can also allow undefined on OSX by adding it to the linker flags ... if you think that's the better solution

BastianZim · 2021-11-03T14:41:29Z

@conda-forge-admin, please rerender

…nda-forge-pinning 2021.11.02.20.53.42

wolfv · 2021-11-03T14:48:50Z

We could add -undefined dynamic_lookup to the macOS builds: https://github.com/search?q=org%3Aconda-forge+dynamic_lookup&type=code

tkralphs · 2021-11-03T14:58:10Z

We can also allow undefined on OSX by adding it to the linker flags ... if you think that's the better solution

That also seems like a bit of a hack. Clp is a direct dependency, it should be linked directly. Didn't we at some point say that all direct dependencies should be listed in the recipe? I guess it was when we were working on CyLP, but didn't we discuss at some point that we need to list all direct dependencies? I just still don't really know why Cgl is the only dependency listed here.

BastianZim · 2021-11-03T15:18:30Z

They were all removed in #6 no idea why. I can add them back if you want?

tkralphs · 2021-11-03T15:48:50Z

OK, I sorted this out. There is a "bug" in the Cbc configure script that was essentially compensated for by the "bug" in the .pc file that we patched. In essence, the configure script itself does not list clp as a direct dependency. This didn't matter before because Clp was not private in OsiClp's .pc file, so it got pulled in anyway because osiclp is listed in Cbc's configure script as a direct dependency.

I can fix this and make a new release of Cbc, but will need a little time. I can also generate a patch for the configure script, but this may be a bit ugly. Although fixing this bug requires only a small patch to configure.ac, it may actually cause a large number of changes in the configure script itself, which is auto-generated. Let me give it a try and see what we do.

tkralphs · 2021-11-03T15:52:23Z

I don't know what the right thing to do is with respect to listing direct dependencies. It shouldn't really matter for success of the build, since Cbc's configure looks for dependencies with pkg-config, which is independent of conda. But I thought there could be problem when downstream project try to link to Cbc if direct dependencies are missing. There are lots of complaints about missing dependencies when I do the build locally. This was the whole Needed DSO ... but not in reqs/run issue that I mentioned earlier. I guess that all direct dependencies should really be listed.

BastianZim · 2021-11-03T16:05:15Z

I guess that all direct dependencies should really be listed.

Do you know them by heart or are they listed somewhere? Happy to add them to this PR.

tkralphs · 2021-11-03T16:07:11Z

The patch is not too bad. It's not a great solution, but messing with the somewhat fragile build system of Cbc 2.10, which will be greatly improved in 3.0, was what we were trying to avoid with the other patches. This is actually just a part of that. Opinions? On the other hand, this can be patched in Cbc itself without too much disruption.

index 2491b662..cd3ff913 100755
--- a/Cbc/configure
+++ b/Cbc/configure
@@ -21484,9 +21484,9 @@ if test $coin_has_clp = notGiven; then


 if test -n "$PKG_CONFIG" ; then
-  if $PKG_CONFIG --exists "osi-clp"; then
-    CLP_VERSIONS=`$PKG_CONFIG --modversion "osi-clp" 2>/dev/null | tr '\n' ' '`
-     cflags=`$PKG_CONFIG --cflags "osi-clp" 2>/dev/null`
+  if $PKG_CONFIG --exists "clp osi-clp"; then
+    CLP_VERSIONS=`$PKG_CONFIG --modversion "clp osi-clp" 2>/dev/null | tr '\n' ' '`
+     cflags=`$PKG_CONFIG --cflags "clp osi-clp" 2>/dev/null`
     # pkg-config cannot handle spaces, so CYGPATH_W cannot be put into .pc files
        # thus, we modify the cflags extracted from pkg-config by putting CYGPATH_W behind -I's
        # but only do this if is not trivial
@@ -21495,8 +21495,8 @@ if test -n "$PKG_CONFIG" ; then
       cflags=`echo $cflags | sed -e 's/-I\([^ ]*\)/-I\`${CYGPATH_W} \1\`/g'`
     fi
     CLP_CFLAGS="$cflags"
-    CLP_LIBS=`$PKG_CONFIG --libs "osi-clp" 2>/dev/null`
-    CLP_DATA=`$PKG_CONFIG --variable=datadir "osi-clp" 2>/dev/null`
+    CLP_LIBS=`$PKG_CONFIG --libs "clp osi-clp" 2>/dev/null`
+    CLP_DATA=`$PKG_CONFIG --variable=datadir "clp osi-clp" 2>/dev/null`
      coin_has_clp=yes
         echo "$as_me:$LINENO: result: yes: $CLP_VERSIONS" >&5
 echo "${ECHO_T}yes: $CLP_VERSIONS" >&6
@@ -21507,22 +21507,22 @@ echo "${ECHO_T}yes: $CLP_VERSIONS" >&6
         then
           CLP_LIBS=`echo " $CLP_LIBS " | sed -e 's/ \(\/[^ ]*\/\)\([^ ]*\)\.lib / \`$(CYGPATH_W) \1 | sed -e "s|\\\\\\\\\\\\\\\\\\\\|\/|g"\`\2.lib /g'`
         fi
-        CLP_PCREQUIRES="osi-clp"
+        CLP_PCREQUIRES="clp osi-clp"

         # augment X_PCREQUIRES, X_CFLAGS, and X_LIBS for each build target X in CbcLib CbcGeneric

-          CBCLIB_PCREQUIRES="osi-clp $CBCLIB_PCREQUIRES"
+          CBCLIB_PCREQUIRES="clp osi-clp $CBCLIB_PCREQUIRES"
           CBCLIB_CFLAGS="$CLP_CFLAGS $CBCLIB_CFLAGS"
           CBCLIB_LIBS="$CLP_LIBS $CBCLIB_LIBS"

-          CBCGENERIC_PCREQUIRES="osi-clp $CBCGENERIC_PCREQUIRES"
+          CBCGENERIC_PCREQUIRES="clp osi-clp $CBCGENERIC_PCREQUIRES"
           CBCGENERIC_CFLAGS="$CLP_CFLAGS $CBCGENERIC_CFLAGS"
           CBCGENERIC_LIBS="$CLP_LIBS $CBCGENERIC_LIBS"



   else
-    CLP_PKG_ERRORS=`$PKG_CONFIG $pkg_short_errors --errors-to-stdout --print-errors "osi-clp"`
+    CLP_PKG_ERRORS=`$PKG_CONFIG $pkg_short_errors --errors-to-stdout --print-errors "clp osi-clp"`
       coin_has_clp=notGiven
         echo "$as_me:$LINENO: result: not given: $CLP_PKG_ERRORS" >&5
 echo "${ECHO_T}not given: $CLP_PKG_ERRORS" >&6
@@ -21558,8 +21558,8 @@ CLP_DATA_INSTALLED=
 CLP_PCLIBS=
 CLP_PCREQUIRES=

-# initial list of dependencies is "osi-clp", but we need to filter out version number specifications (= x, <= x, >= x, != x)
-projtoprocess="osi-clp"
+# initial list of dependencies is "clp osi-clp", but we need to filter out version number specifications (= x, <= x, >= x, != x)
+projtoprocess="clp osi-clp"

 # we first expand the list of projects to process by adding all dependencies just behind the project which depends on it
 # further, we collect the list of corresponding .pc files, but do this in reverse order, because we need this order afterwards
@@ -21760,15 +21760,15 @@ _ACEOF
     CLP_LIBS_INSTALLED=`echo " $CLP_LIBS_INSTALLED" | sed -e 's/ \(\/[^ ]*\/\)/ \`$(CYGPATH_W) \1\`/g' -e 's/ -l\([^ ]*\)/ lib\1.lib/g' -e 's/ -L\([^ ]*\)/ -libpath:\`$(CYGPATH_W) \1\`/g'`
   fi

-  CLP_PCREQUIRES="osi-clp"
+  CLP_PCREQUIRES="clp osi-clp"

-    CBCLIB_PCREQUIRES="osi-clp $CBCLIB_PCREQUIRES"
+    CBCLIB_PCREQUIRES="clp osi-clp $CBCLIB_PCREQUIRES"
     CBCLIB_CFLAGS="$CLP_CFLAGS $CBCLIB_CFLAGS"
     CBCLIB_LIBS="$CLP_LIBS $CBCLIB_LIBS"
     CBCLIB_CFLAGS_INSTALLED="$CLP_CFLAGS_INSTALLED $CBCLIB_CFLAGS_INSTALLED"
     CBCLIB_LIBS_INSTALLED="$CLP_LIBS_INSTALLED $CBCLIB_LIBS_INSTALLED"

-    CBCGENERIC_PCREQUIRES="osi-clp $CBCGENERIC_PCREQUIRES"
+    CBCGENERIC_PCREQUIRES="clp osi-clp $CBCGENERIC_PCREQUIRES"
     CBCGENERIC_CFLAGS="$CLP_CFLAGS $CBCGENERIC_CFLAGS"
     CBCGENERIC_LIBS="$CLP_LIBS $CBCGENERIC_LIBS"
     CBCGENERIC_CFLAGS_INSTALLED="$CLP_CFLAGS_INSTALLED $CBCGENERIC_CFLAGS_INSTALLED"

tkralphs · 2021-11-03T16:19:48Z

They were all removed in #6 no idea why. I can add them back if you want?

Yes, in fact, there was a whole discussion of this at the time and I said then that these were indirect dependencies, but I don't think I had the right concept of "direct" versus "indirect" then and why it matters for conda in terms of linking/overlinking. I was even less knowledgeable about conda then. So I think I was right to question this, but wrong to conclude that it was OK to remove these dependencies. But again, this is unrelated to the failure and won't fix it.

Do you know them by heart or are they listed somewhere? Happy to add them to this PR.

Yes, they would be as I think they already were before.

coin-or-cgl
coin-or-clp
coin-or-osi
coin-or-coinutils

BastianZim · 2021-11-03T16:30:58Z

patch

Would defer to @wolfv here.

But again, this is unrelated to the failure and won't fix it.

I think it still makes sense to add them back just to have everything included and ready to go for the new version.

Yes, they would be as I think they already were before.

Based on jschueller@d5e365f they were:

host:
    - coin-or-utils
    - coin-or-cgl
    - coin-or-clp
    - coin-or-osi
    - libblas
    - libcblas
    - liblapack
    - readline  # [unix]
    - zlib
    - bzip2
    # mumps and nauty are optional deps. currently outdated, leading to solver issues on osx
    # - mumps
    # - nauty
    - ncurses
    # TODO upstream should link with blas, not openblas
    - openblas
  run:
    - ncurses
    - openblas

Any other besides the coin-or ones that should be added?

tkralphs · 2021-11-03T16:50:21Z

Any other besides the coin-or ones that should be added?

Hmm, I don't think any of the others are direct dependencies of Cbc.

BastianZim · 2021-11-03T17:45:43Z

Patch works! 🎉

BastianZim · 2021-11-03T17:47:20Z

@wolfv If you merge this, would mind archiving https://github.com/conda-forge/coincbc-feedstock as well? Thanks!

conda-forge/conda-forge-pinning-feedstock#2080 can also be merged then.

tkralphs · 2021-11-03T21:24:45Z

Patch works! 🎉

Great! It's not pretty, but glad we got it sorted.

BastianZim · 2021-11-03T23:31:51Z

Thank you!

BastianZim added 3 commits October 30, 2021 19:51

Make multi output

b422db3

Add coincbc

4964ae3

Fix indent

3b7b4c1

Make name explicit

05fe8cf

BastianZim added 2 commits October 30, 2021 20:06

Add description to meta package

ae65aac

Add test

e31f71a

BastianZim marked this pull request as ready for review October 30, 2021 18:08

BastianZim requested review from tkralphs and wolfv as code owners October 30, 2021 18:08

MNT: Re-rendered with conda-build 3.21.4, conda-smithy 3.14.1, and co…

0a390df

…nda-forge-pinning 2021.10.30.17.45.23

This was referenced Oct 30, 2021

Add coin-or stack conda-forge/conda-forge-pinning-feedstock#2080

Merged

Split package conda-forge/coincbc-feedstock#37

Open

isuruf and others added 2 commits October 30, 2021 13:39

Fix metadata

e1dddcc

Rename build script

6b4fe72

BastianZim mentioned this pull request Oct 31, 2021

Add to pinning conda-forge/or-tools-feedstock#3

Closed

tkralphs mentioned this pull request Nov 3, 2021

Fix patch for 1.17.4 conda-forge/coin-or-clp-feedstock#9

Closed

5 tasks

BastianZim mentioned this pull request Nov 3, 2021

Add second patch and min pin conda-forge/coin-or-clp-feedstock#6

Merged

5 tasks

Add coin-or-clp

b91324f

MNT: Re-rendered with conda-build 3.21.4, conda-smithy 3.14.2, and co…

9dbe332

…nda-forge-pinning 2021.11.02.20.53.42

BastianZim added 3 commits November 3, 2021 17:58

Add deps back

506922b

Fix utils name

32b59c2

Add build patch

7ca6bb6

wolfv merged commit 89d4dfc into conda-forge:master Nov 3, 2021

BastianZim deleted the add-coincbc branch November 3, 2021 23:32

tkralphs mentioned this pull request Nov 30, 2021

BUG: Error when using CyLP ARM-DOE/pyart#1022

Open

Add coincbc #11

Add coincbc #11

Conversation

BastianZim commented Oct 30, 2021 • edited Loading

conda-forge-linter commented Oct 30, 2021

conda-forge-linter commented Oct 30, 2021

conda-forge-linter commented Oct 30, 2021

BastianZim commented Oct 30, 2021

BastianZim commented Oct 30, 2021

BastianZim commented Oct 30, 2021

github-actions bot commented Oct 30, 2021

BastianZim commented Nov 1, 2021

wolfv commented Nov 2, 2021

wolfv commented Nov 2, 2021 • edited Loading

BastianZim commented Nov 2, 2021

wolfv commented Nov 2, 2021

isuruf commented Nov 2, 2021

BastianZim commented Nov 2, 2021

tkralphs commented Nov 2, 2021

tkralphs commented Nov 3, 2021

BastianZim commented Nov 3, 2021

BastianZim commented Nov 3, 2021

tkralphs commented Nov 3, 2021 • edited Loading

wolfv commented Nov 3, 2021

BastianZim commented Nov 3, 2021

wolfv commented Nov 3, 2021

tkralphs commented Nov 3, 2021

BastianZim commented Nov 3, 2021

tkralphs commented Nov 3, 2021

tkralphs commented Nov 3, 2021

BastianZim commented Nov 3, 2021

tkralphs commented Nov 3, 2021 • edited Loading

tkralphs commented Nov 3, 2021

BastianZim commented Nov 3, 2021

tkralphs commented Nov 3, 2021

BastianZim commented Nov 3, 2021

BastianZim commented Nov 3, 2021 • edited Loading

tkralphs commented Nov 3, 2021

BastianZim commented Nov 3, 2021

BastianZim commented Oct 30, 2021 •

edited

Loading

wolfv commented Nov 2, 2021 •

edited

Loading

tkralphs commented Nov 3, 2021 •

edited

Loading

tkralphs commented Nov 3, 2021 •

edited

Loading

BastianZim commented Nov 3, 2021 •

edited

Loading