-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
@mxnet-label-bot add [pr-awaiting-review] |
Seems like this time the CI is reporting an actual problem. @wkcn could you take a look? |
@szha Yes. OpenCV2 does not have libopencv_imgcodecs.so, but OpenCV3/4 need it. I would like to add the variable OPENCV_DEPS_PATH, then check whether $(OPENCV_DEPS_PATH)/lib/libopencv_imgcodecs.(a|so) exists. |
I do not know what OpenCV shared library we did not link. It raises undefined reference in unit-cpu CI. |
@wkcn it might be that the system has an opencv2 installation while the build is statically linking opencv3. |
@szha I see. I used wrong linking order. I will fix it. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the compatibility fix and the improvement 💯
As far as I know, when I tried to build mxnet on win10 with cuda 10.1. It failed with compling error.
|
@dbsxdbsx Could you please open an issue and provide more error message? |
CFLAGS += -DMXNET_USE_OPENCV=1 $(shell pkg-config --cflags opencv) | ||
LDFLAGS += $(filter-out -lopencv_ts, $(shell pkg-config --libs opencv)) | ||
CFLAGS += -DMXNET_USE_OPENCV=1 | ||
ifneq ($(USE_OPENCV_INC_PATH), NONE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be problematic when USE_OPENCV_INC_PATH
is not defined in config.mk
, because it's an empty string at the time. In that situation, no correct paths to headers and libs of opencv are actually added to CFLAGS
and LDFLAGS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reminisce good catch. @wkcn how about we add a default value for empty value here to help with cases where people already have old config.mk without the new variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reminisce @szha Hi! I have fixed it in the PR #14424
* compatibility with opencv4 * update makefile * update Makefile * fix Makefile * fix lib path * update make.mk * add -lopencv_highgui * retrigger CI * update makefile * remove libs-L * Fix OpenCV linking order * try to fix the bug of linking static libraries
* compatibility with opencv4 * update makefile * update Makefile * fix Makefile * fix lib path * update make.mk * add -lopencv_highgui * retrigger CI * update makefile * remove libs-L * Fix OpenCV linking order * try to fix the bug of linking static libraries
* compatibility with opencv4 * update makefile * update Makefile * fix Makefile * fix lib path * update make.mk * add -lopencv_highgui * retrigger CI * update makefile * remove libs-L * Fix OpenCV linking order * try to fix the bug of linking static libraries
Description
To compatible with multiple versions of OpenCV, define the old OpenCV macros to support OpenCV4.
In OpenCV4,
opencv2/imgproc/imgproc_c.h
andopencv2/videoio/videoio_c.h
define these macros, however I couldn't include them since it would bring redefinition problems.Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
src/io/opencv_compatibility.h
to define the OpenCV macros.Makefile
to find the path ofopencv4
Comments
There is a similar PR #13559, Thank @daleydeng