-
Notifications
You must be signed in to change notification settings - Fork 660
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
Do not modify source directory during build #270
base: devel
Are you sure you want to change the base?
Conversation
Here's the related issue and previous discussion: #133 |
...? |
Ugh. Okay, for testing, I'd deleted source tree artifacts. Apparently, that gets built at configure time? Yuck. It's also still in the source tree:
...but at least things seem better with this PR than without... |
Hi @mwoehlke-kitware, Looking forward to get your expertise and insights on next steps as well :-). Best wishes, |
Yes, the inclusion / build of the bundled QGLViewer is still quite messy. Currently the Find... script tries to find or build it (related issue: #225). Nowadays, an ExternalProjct would be the best solution, which was not available back then. Due to the common availability of QGLViewer, we could also just remove the bundled version. |
I'm not sure even ExternalProject is necessary... the pattern I usually see is to have an option whether to use the internal version or not. (A failed attempt to find an external version might set the default value of this.) If using the internal version, just add it as another target to the build. Roughly¹: find_package(QGLViewer)
if(QGLViewer_FOUND)
set(_use_qglviewer_default OFF)
else()
set(_use_qglviewer_default OFF)
endif()
option(OCTOMAP_USE_BUNDLED_QGLVIEWER "..." ${_use_qglviewer_default})
if(OCTOMAP_USE_BUNDLED_QGLVIEWER)
set(QGLViewer_LIBRARIES QGLViewer)
add_subdirectory(thirdparty/QGLViewer)
else()
find_package(QGLViewer REQUIRED)
endif() It's actually very difficult to (correctly) use external projects to build dependencies unless you use the "superbuild" pattern. (¹ This is off the top of my head without checking the sources, and should be considered pseudo-code. In particular, variable names may be wrong.) |
This separates out two commits from #269. No need to merge them now, but to keep open for inclusion in next minor release.