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

enable build in vscode, custom toolchain+include #258

Merged
merged 2 commits into from
Nov 20, 2021

Conversation

diablodale
Copy link
Contributor

fixes #246

full details on how this fix can be used in vscode is in the above issue.

Comment on lines 66 to 69
# include optional dependency cmake
if(DEFINED DEPTHAI_DEPENDENCY_INCLUDE)
include(${DEPTHAI_DEPENDENCY_INCLUDE} OPTIONAL)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

depthaiDependencies.cmake are installed as well to specify correctly all dependencies.
Any requirement to carry over the specified include over to be installed as well?

Otherwise as its guarded LGTM for non-installed use case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

depthaiDependencies.cmake is part of the cmake by a fixed include() and a git tracked file. Therefore, it is more troublesome to customize it (always have to merge, conflicts, etc.)...separating the upstream from the local customize via vars. One is the toolchain (already done), second and third are dependencies and libraries (mostly for opencv).

Sorry, I don't understand your question. Could you word it another way?

Also, I've been considering if the two I added THIRDPARTY_OPENCV_LIBRARIES and DEPTHAI_DEPENDENCY_INCLUDE should have option() for them. If you think good to have, then I can update the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Worded it oddly - meant to ask if the DEPTHAI_DEPENDENCY_INCLUDE file should be installed as well, so that consumers get this information as well, or should this be for local use only?

Yeah, I think it'd be good for discoverability. One a path and one a string type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I slightly lean towards not installing the file DEPTHAI_DEPENDENCY_INCLUDE.

  • in my specific usage, it is only extra stuff needed to compile depthai and its test/examples. For example, I use TBB, IPP, and Eigen with my private OpenCV. So I need to find_package() those so that later cmake can correctly find includes and link EXEs.
  • I tend to think that the devs that need this (a minority) will privately manage their DEPTHAI_DEPENDENCY_INCLUDE file for depthai itself -and- also for their final EXE. In my case, I have a cmake for my final EXE which manages its own find_package(), points to the depthai cmake, etc. For example, I need OpenCV directly in my EXE so I find_package() for TBB, IPP, and Eigen in the app's cmake. So I tend to think a dev needing this will self-manage and that they only need a way to adapt depthai without having to make private commits to it.

I'll add options for the two and force push after this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good - lets leave it for local only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed, ci is running

Copy link
Collaborator

@themarpe themarpe left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@diablodale
Copy link
Contributor Author

I don't think this is a me/code issue. It complains of

Couldn't check if depthai-shared codebase matches between device and host

@themarpe themarpe changed the base branch from main to develop November 20, 2021 02:27
@themarpe themarpe merged commit 98bbf85 into luxonis:develop Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fails to cmake config+build on Windows using cmake and vscode (hunter pain)
2 participants