re-enable scip

[h-vetinari]

See discussion in #2

[conda-forge-webservices[bot]]

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.

[h-vetinari]

So something is off with FindSCIP - it doesn't find our CMake metadata for scip, and then insists on setting SCIP_ROOT, which I don't want to impose on all consumers. First I thought it's the NO_MODULE, then the fact that our scip target seems to be spelled lower-case, but neither of those suspicions turned out correct.

[h-vetinari]

@Mizux, this looks pretty good now. Is there a specific way we should test the scip-integration?

[Mizux]

Try to build and run this sample since is using the SCIP solver https://github.com/google/or-tools/blob/main/ortools/linear_solver/samples/simple_mip_program.cc

warning: if scip unavailable it will return silently so you can hack these lines to make it crash instead if SCIP solver can't be loaded... note: I guess a LOG(FATAL) could also be a good solution ;) https://github.com/google/or-tools/blob/471a45dd550e4ae33d46c16261eb3e326d2c4864/ortools/linear_solver/samples/simple_mip_program.cc#L27-L30

[h-vetinari]

@Mizux - good news, scip integration is working. Unfortunately I couldn't just build https://github.com/google/or-tools/blob/main/ortools/linear_solver/samples directly (missing CMake function add_cxx_sample, and local cmake/cpp.cmake depends on a lot of things), so it's harder to just do this as an integration test.

How heavy is the BUILD_SAMPLES=ON footprint? Would it make sense to distribute them (that way it would be way easier to test them; corresponds to the current state of this PR), perhaps as a separate output libortools-samples (doesn't yet correspond to this PR)?

Would appreciate your comments. :)

[h-vetinari]

Hey @Mizux, hope you had a nice holiday. Once you dig through your notifications, could you please let me know your thoughts about the above?

[h-vetinari]

How heavy is the BUILD_SAMPLES=ON footprint? Would it make sense to distribute them (that way it would be way easier to test them; corresponds to the current state of this PR), perhaps as a separate output libortools-samples (doesn't yet correspond to this PR)?

Would appreciate your comments. :)

Ping @Mizux

[Mizux]

How heavy is the BUILD_SAMPLES=ON footprint? Would it make sense to distribute them (that way it would be way easier to test them; corresponds to the current state of this PR), perhaps as a separate output libortools-samples (doesn't yet correspond to this PR)? Would appreciate your comments. :)

Ping @Mizux

Hi, sorry for the delay enabling samples will produce ~90 binaries of ~30ko each, except few of them which are 2Mo. After investigation it seems that any or-tools samples (mostly routing ones) which deal with a Protobuf Duration variable will get this 2Mo (note: we are doing some static build of protobuf) so in conda if you are using a protobuf shared deps you shouldn't have this issue.

dev note:

diff --git a/ortools/constraint_solver/samples/vrp_drop_nodes.cc b/ortools/constraint_solver/samples/vrp_drop_nodes.cc
index efcea93d1e..15a47891c5 100644
--- a/ortools/constraint_solver/samples/vrp_drop_nodes.cc
+++ b/ortools/constraint_solver/samples/vrp_drop_nodes.cc
@@ -185,7 +185,7 @@ void VrpDropNodes() {
       FirstSolutionStrategy::PATH_CHEAPEST_ARC);
   search_parameters.set_local_search_metaheuristic(
       LocalSearchMetaheuristic::GUIDED_LOCAL_SEARCH);
-  search_parameters.mutable_time_limit()->set_seconds(1);
+  //search_parameters.mutable_time_limit()->set_seconds(1);
   // [END parameters]

commenting this line remove the 2Mo extra cost i.e. sample don't pull libprotobuf.a stuff anymore...

concerning the install:

1. our "create sample" CMake function helper will add it to the install, we could add a new option INSTALL_SAMPLES/INSTALL_EXAMPLES etc to opt out the install (i.e. be able to build them, run them but won't be part of the install export)

2. We could also try to add a "COMPONENT" parameter to the install() call. see: https://cmake.org/cmake/help/latest/command/install.html#introduction

https://github.com/google/or-tools/blob/7a9180f625f52aeccb94c6215705c9d74c000ded/cmake/cpp.cmake#L457-L458

[h-vetinari]

Thanks for the reply!

That's a pretty big footprint just for an integration test. Would it be possible to enable building just a single sample?

Unfortunately I couldn't just build https://github.com/google/or-tools/blob/main/ortools/linear_solver/samples directly (missing CMake function add_cxx_sample, and local cmake/cpp.cmake depends on a lot of things), so it's harder to just do this as an integration test.

[h-vetinari]

commenting this line remove the 2Mo extra cost i.e. sample don't pull libprotobuf.a stuff anymore...

Actually, we're not using static protobuf at all, but dynamic one, so we wouldn't "pay" that cost. 🤔

[h-vetinari]

@Mizux

CI here has started failing (also in other PRs like #42, even for the same protobuf version that used to pass before), with a redeclaration of google::protobuf::AbslStringify. The code comment makes it sound like this is an interface that needs to be reimplemented (and indeed the build fails much harder if it's removed), but now I don't know how to fix it. Thoughts?

$SRC_DIR/ortools/base/logging.h:60:6: error: redefinition of 'template<class Sink> void google::protobuf::AbslStringify(Sink&, const Message&)'
   60 | void AbslStringify(Sink& sink, const Message& msg) {
      |      ^~~~~~~~~~~~~
In file included from $SRC_DIR/ortools/base/logging.h:29:
$BUILD_PREFIX/include/google/protobuf/message.h:342:15: note: 'template<class Sink> void google::protobuf::AbslStringify(Sink&, const Message&)' previously declared here
  342 |   friend void AbslStringify(Sink& sink, const google::protobuf::Message& message) {
      |               ^~~~~~~~~~~~~

[Mizux]

@Mizux

CI here has started failing (also in other PRs like #42, even for the same protobuf version that used to pass before), with a redeclaration of google::protobuf::AbslStringify. The code comment makes it sound like this is an interface that needs to be reimplemented (and indeed the build fails much harder if it's removed), but now I don't know how to fix it. Thoughts?

$SRC_DIR/ortools/base/logging.h:60:6: error: redefinition of 'template<class Sink> void google::protobuf::AbslStringify(Sink&, const Message&)'
   60 | void AbslStringify(Sink& sink, const Message& msg) {
      |      ^~~~~~~~~~~~~
In file included from $SRC_DIR/ortools/base/logging.h:29:
$BUILD_PREFIX/include/google/protobuf/message.h:342:15: note: 'template<class Sink> void google::protobuf::AbslStringify(Sink&, const Message&)' previously declared here
  342 |   friend void AbslStringify(Sink& sink, const google::protobuf::Message& message) {
      |               ^~~~~~~~~~~~~

My bad, when trying to sync our internal base with github export I must have introduced this bug (again ? pretty sure I already I fixed it few weeks ago ><; ) Need to check two things: std::string vs std::string_view (IIRC protobuf/absl don't use the same type internally vs externally -> need to patch our export...)

[h-vetinari]

That failure wasn't there a few weeks ago, and I'm still building the same version, so it seems it's something else. I have some suspicion that it might be related to how the python bindings here are rebuilding the c++ bits (but then not packaging them?! no idea how the packages were functional so far) - I'm trying to address this in #43