Rebuild for protobuf423 with additional fixes

[traversaro]

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.

[conda-forge-webservices]

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]

The additional fixes seems to be working fine.

[traversaro]

@conda-forge-admin, please rerender

[hmaarrfk]

is this stuff necessary still?

[traversaro]

I removed and almost all jobs are passing, so I guess no.

[hmaarrfk]

yeah, i'm somewhat worried it is necessary for osx lol ;) you never know!

[hmaarrfk]

i'm somewhat worried it is due to the order of arguments.

I think protobuf now expeoses abseil (right???) and thus needs a consistent version of c++ between libraries.

this is why I originally had added it (before your patch)

[hmaarrfk]

it may be that for all other platforms except for osx arm64, the order of the c++ flags makes it so that it is c++17 and not c++11

[traversaro]

The osx-arm64 failure is due to the use of the host protoc instead of the build one:

2023-06-05T08:59:56.3610410Z [552/1488] cd $SRC_DIR/build/modules/dnn && $PREFIX/bin/protoc-23.2.0 --cpp_out :$SRC_DIR/build/modules/dnn -I $SRC_DIR/modules/dnn/src/caffe -I $SRC_DIR/modules/dnn/src/onnx -I $SRC_DIR/modules/dnn/src/tensorflow $SRC_DIR/modules/dnn/src/caffe/opencv-caffe.proto
2023-06-05T08:59:56.3612450Z FAILED: modules/dnn/opencv-caffe.pb.h modules/dnn/opencv-caffe.pb.cc $SRC_DIR/build/modules/dnn/opencv-caffe.pb.h $SRC_DIR/build/modules/dnn/opencv-caffe.pb.cc 
2023-06-05T08:59:56.3614230Z cd $SRC_DIR/build/modules/dnn && $PREFIX/bin/protoc-23.2.0 --cpp_out :$SRC_DIR/build/modules/dnn -I $SRC_DIR/modules/dnn/src/caffe -I $SRC_DIR/modules/dnn/src/onnx -I $SRC_DIR/modules/dnn/src/tensorflow $SRC_DIR/modules/dnn/src/caffe/opencv-caffe.proto
2023-06-05T08:59:56.3615580Z /bin/sh: $PREFIX/bin/protoc-23.2.0: Bad CPU type in executable

see conda-forge/conda-forge-pinning-feedstock#4075 (comment) for a possible patch. I do not think there is any problem of c++ flags order, if you look to the compiler invocation each invocation either has -std=c++11 or -std=c++17. As protobuf or abseil are not exposed in public headers, there is no risk of this creating problems. However, if we want to just play safe, we can re-add the patch to ensure that all of opencv compilers with C++17.

[traversaro]

The osx-arm64 failures hopefully will be fixed by 2436f99 .

[h-vetinari]

if you look to the compiler invocation each invocation either has -std=c++11 or -std=c++17

nope nope nope nope, please let's not play this game of whack-a-mole with abseil (which only supports a consistent C++ standard). 🙏

As protobuf or abseil are not exposed in public headers

They've mentioned they plan to use abseil in their public API, so this will happen eventually.

[h-vetinari]

They've mentioned they plan to use abseil in their public API, so this will happen eventually.

Ah sorry, I got mixed up here. Protobuf will expose abseil in its API, opencv may choose not to do that. Still, I'd strongly advise that we use a uniform C++ standard version here.

[traversaro]

if you look to the compiler invocation each invocation either has -std=c++11 or -std=c++17

nope nope nope nope, please let's not play this game of whack-a-mole with abseil (which only supports a consistent C++ standard). 🙏

Sure, I was just replying to @hmaarrfk on the point of the order of the arguments, not endorsing mixing standard versions in the same library (that unfortunatly it is what happens if one set manually CMAKE_CXX_STANDARD and/or CXX_STANDARD .

[traversaro]

Windows failures were probably fixed by conda-forge/libprotobuf-feedstock#162 .

[traversaro]

I think the PR is ready for review, the only two remaining failures are temporary network failures.

[hmaarrfk]

Hmmm. Seems good to me

[h-vetinari]

One small thing, otherwise LGTM!

[h-vetinari]

Completely unrelated, but small enough to consider it nevertheless: could you add dev_url: https://github.com/opencv/opencv under extras:? I find it nice to be able to navigate to the upstream repo directly from the feedstock README.

[traversaro]

Completely unrelated, but small enough to consider it nevertheless: could you add dev_url: https://github.com/opencv/opencv under extras:? I find it nice to be able to navigate to the upstream repo directly from the feedstock README.

Done: 0011f24 .

[traversaro]

@conda-forge-admin, please rerender

[mshabunin]

I'm wondering whether it is really necessary to raise C++ standard level. Conda packages can be used by C++ developers and sudden change of C++ ABI can break their applications. Does conda-forge have predefined set of toolchains which have stable ABI for all packages in the repository? OpenCV wouldn't override C++ standard, but it will set it to 11 if it was not explicitly defined.

It means that OpenCV have failed to find Lapack-compatible library (OpenBLAS, MKL, ...):

CMake Error at cmake/OpenCVUtils.cmake:797 (message):
  Some dependencies have not been found or have been forced, unset
  ENABLE_CONFIG_VERIFICATION option to ignore these failures or change
  following options:

  WITH_LAPACK

Technically Lapack is not required and can be disabled if it gives troubles on this platform (explicitly set WITH_LAPACK=OFF). Package dependencies will be changed in this case though, I'm not sure if it's a good thing.

[h-vetinari]

I'm wondering whether it is really necessary to raise C++ standard level.

Yes, because of abseil, see here.

Conda packages can be used by C++ developers and sudden change of C++ ABI can break their applications.

Where we know of ABI breaks based on the standard version (like abseil), we take great care to rebuild all dependent packages so that everything in conda-forge uses a consistent ABI. If people compile C++ on top of conda-forge dependencies and update any ABI-relevant library in their environment (basically anything in the global pinning), it's up to them to recompile (effectively a local execution of our migration machinery).

Does conda-forge have predefined set of toolchains which have stable ABI for all packages in the repository?

The answer is yes, but (aside from compilers and fundamental support libraries) that baseline is constantly in flux, as new versions with new ABIs get released, and we migrate all dependents over to that, before we upgrade the baseline to that. You can see the ongoing migrations here.

As I mentioned, abseil is special here. And the problem is made worse by protobuf using abseil in their public API. This also makes the protobuf migration special - because we expect problems along the way, we actually double the build matrix on all feedstocks, until we can build everything with protobuf 4 (normal migrations rely on the assumption that they can be done relatively quickly across the ecosystem, which is not the case here)

[mshabunin]

Thank you for reply. Currently I can see that the only issue is missing Lapack dependency, it could be that try_compile in the OpenCV here is missing <LANG>_STANDARD <std> or can not be compiled using C++ 17. I suggest removing all sed patching and checking <build>/CMakeFiles/CMakeError.log to determine what is wrong with Lapack. Alternatively we can disable Lapack temporarily and reintroduce it later.

[h-vetinari]

Alternatively we can disable Lapack temporarily and reintroduce it later.

I think we should get lapack to work again. Not sure what broke though. In any case, Lapack is Fortran (and a C interface), so shouldn't be affected by the C++ standard. More than that, it shouldn't be rebuilt at all (if we're talking about the actual Lapack and not OpenCV's interface to it), but taken from the one we have already.

[mshabunin]

FYI, I've been able to build OpenCV with protobuf@v3.23.3 with minor OpenCV patching and raising CXX_STANDARD to 17 (opencv/opencv#23791 (comment)). Official new protobuf support in the upstream will require slightly more effort though.

[traversaro]

FYI, I've been able to build OpenCV with protobuf@v3.23.3 with minor OpenCV patching and raising CXX_STANDARD to 17 (opencv/opencv#23791 (comment)). Official new protobuf support in the upstream will require slightly more effort though.

A related patch can be found in https://github.com/conda-forge/opencv-feedstock/blob/1925cac9c12099a6c710eee3f79c847f76166f75/recipe/fix_protobuf23.patch .

[h-vetinari]

Here's the aggregate of my changes. Hopefully the commits make sense individually. I've picked a few obvious improvements from MicrosoftDocs/cpp-docs#365 as well.

[h-vetinari]

Not sure where this is coming from now; perhaps because I'm triggering not just on error-code 1, but NEQ 0? 🤔

Verifying WITH_IPP=ON => 'HAVE_IPP'=FALSE

[h-vetinari]

Pretty sure that wasn't me (osx / win):

  File "C:\Miniforge\lib\site-packages\conda_index\index\__init__.py", line 347, in _get_resolve_object
    sd._process_raw_repodata(repodata_copy)  # type: ignore
TypeError: SubdirData._process_raw_repodata() missing 1 required positional argument: 'state'

[h-vetinari]

Aside from the frankly unsustainable build matrix (luckily, we can improve this soon with conda-forge/conda-forge-pinning-feedstock#4605 and/or #365), this LGTM now.

Great sleuthing on the C++17 vs. LAPACK issue, resp. the one about finding protobuf!

PS. Happy to give this some more time if someone wants to check my patch rebase butchery (though hopefully, the outcome is tasty 😋).

[hmaarrfk]

Alright great! Thanks all! very exciting!