Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[darknet] Fix dependence cuda #35413

Merged
merged 18 commits into from
Dec 12, 2023
Merged

Conversation

FrankXie05
Copy link
Contributor

Fix #33355

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@FrankXie05 FrankXie05 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Nov 30, 2023
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the feature dependencies added, isn't the patch obsolete now?

Comment on lines 7 to 10
if(ENABLE_OPENCV)
find_package(OpenCV REQUIRED)
- if(OpenCV_FOUND)
+ if(ENABLE_OPENCV)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointless. The block starts with if(ENABLE_OPENCV).
And given REQUIRED, ENABLE_OPENCV implies OpenCV_FOUND.
(However, NOT ENABLE_OPENCV doesn't necessarily imply NOT OpenCV_FOUND.)

ports/darknet/fix-opencv-cuda.patch Outdated Show resolved Hide resolved
@cenit
Copy link
Contributor

cenit commented Dec 4, 2023

being the author of the original CMakeLists.txt, can you please tell me the errors and the logic behind your fixes?
So that we can properly fix also upstream... :)

@FrankXie05
Copy link
Contributor Author

@cenit Thank you for your reply,
The core problem is:
Condition if(OpenCV_FOUND AND OpenCV_VERSION VERSION_GREATER "3.0" AND BUILD_USELIB_TRACK) cannot be satisfied.
https://github.com/AlexeyAB/darknet/blob/27b37bf1033c1524121850f3705044f50c0c1b8c/CMakeLists.txt#L468

Because OpenCV_FOUND cannot be provided and found.

@dg0yt
Copy link
Contributor

dg0yt commented Dec 5, 2023

Because OpenCV_FOUND cannot be provided and found.

There already is find_package(OpenCV REQUIRED).

AFAIU there is an issue with finding CUDA. Which at least has the effect of turning off BUILD_USELIB_TRACK.

@cenit
Copy link
Contributor

cenit commented Dec 5, 2023

Because OpenCV_FOUND cannot be provided and found.

There already is find_package(OpenCV REQUIRED).

AFAIU there is an issue with finding CUDA. Which at least has the effect of turning off BUILD_USELIB_TRACK.

dg0yt is right.
The condition is mandatory for uselib_track to be built, otherwise you are trying to build something without mandatory dependencies. It really needs an opencv newer than 3.0 and it needs also some verifications on CUDA, which are performed before and that might turn off the "BUILD_USELIB_TRACK"

@cenit
Copy link
Contributor

cenit commented Dec 6, 2023

the pr is still broken from my point of view.
The feature opencv-cuda is not really related to darknet[cuda,cudnn], so there is no reason to enable it if the user only wanted a darknet with an opencv-cuda enabled backend... Is it fixing now any kind of problem? Again, may I know what was the problem?

@FrankXie05
Copy link
Contributor Author

@cenit
./vcpkg install darknet[opencv-cuda]

CMake Error at scripts/cmake/vcpkg_copy_tools.cmake:36 (message):
Couldn't find tool "uselib_track":

  "E:/github/vcpkg/packages/darknet_x64-windows/bin/uselib_track.exe" does not exist

@FrankXie05 FrankXie05 closed this Dec 7, 2023
@FrankXie05 FrankXie05 reopened this Dec 7, 2023
@cenit
Copy link
Contributor

cenit commented Dec 7, 2023

so the error is just at the copy tool clause: to find the uselib_track tool built, you need to have selected opencv-cuda feature AND cuda+cudnn feature, not just opencv-cuda feature.
I think that's the only fix to do

@FrankXie05 FrankXie05 marked this pull request as ready for review December 7, 2023 06:05
@LilyWangLL LilyWangLL marked this pull request as draft December 7, 2023 07:49
{
"name": "darknet",
"features": [
"cuda"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cenit
I think we only need to add cuda. Do we need cudnn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. For the editing of your comments. I seem to have made a mistake in my operation. 😅

@FrankXie05
Copy link
Contributor Author

so the error is just at the copy tool clause: to find the uselib_track tool built, you need to have selected opencv-cuda feature AND cuda+cudnn feature, not just opencv-cuda feature.
I think that's the only fix to do

Judging from the code logic of CMakeLists.txt, it is just the call of macro ENABLE_CUDA that is missing.
Feature darknet[opencv-cuda, opencv3-cuda] only relies on darknet[cuda].

@cenit
Copy link
Contributor

cenit commented Dec 8, 2023

without the cuda feature of course you cannot have enable_cuda on!
the error is on the vcpkg port, which is expecting the uselib_track tool when only the opencv-cuda is enabled. But that feature does not enable cuda in darknet, just in its opencv dependency. To enable cuda in darknet you have to request the cuda feature. And uselib_track will be built when both opencv-cuda and cuda features are enabled!

@FrankXie05 FrankXie05 marked this pull request as ready for review December 11, 2023 02:31
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Dec 11, 2023
@vicroms vicroms merged commit 3f3d8b0 into microsoft:master Dec 12, 2023
15 checks passed
@FrankXie05 FrankXie05 deleted the dev/Frank/33355 branch December 13, 2023 06:07
@cenit
Copy link
Contributor

cenit commented Dec 15, 2023

but but... the result should have been different, as i tried many times to explain... uselib_track should have been copied as a tool when both feature were enabled, otherwise it does not exist by definition!
The result now is illogic, because you made one feature depending on the other (which makes one of the two superfluous...)
@vicroms

@FrankXie05
Copy link
Contributor Author

However, this condition is not enough to open BUILD_USELIB_TRACK. (missing ENABLE_CUDA) .
("opencv-cuda" IN_LIST FEATURES OR "opencv3-cuda" IN_LIST FEATURES)
https://github.com/AlexeyAB/darknet/blob/27b37bf1033c1524121850f3705044f50c0c1b8c/CMakeLists.txt#L326

@cenit
Copy link
Contributor

cenit commented Dec 17, 2023

@FrankXie05 I wrote that file (the cmakelists you're linking)...
Maybe I am not able to explain clearly enough! But the corrections should have been on the portfile, at L44: the uselib_track exists only when "both" cuda AND opencv-cuda features are selected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[darknet] Build error
5 participants