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

Modernize CMake build scripts #3456

Merged
merged 1 commit into from
Jun 4, 2021
Merged

Modernize CMake build scripts #3456

merged 1 commit into from
Jun 4, 2021

Conversation

stotko
Copy link
Contributor

@stotko stotko commented May 17, 2021

  • Split collecting source files into subdirectories: Only single layer (e.g. add_subdirectory(core)) with all further subdirectories handled inside that layer (e.g. core/hashmap in core/CMakeLists.txt)
  • Use target_sources(<target> PRIVATE ...) to specify source files for <target>
  • Sort source files by name and split them into groups based on their (sub)directory within the layer
  • Remove GLOB statements and list all source files explicitly
  • Avoid variables and set properties directly for better readability
  • Consistent function naming convention open3d_<function_name>(...)
  • Link against Open3D::Open3D instead of ${PROJECT_NAME}

This change is Reviewable

@stotko stotko requested review from yxlao, prewettg, errissa and ssheorey May 17, 2021 15:11
@update-docs
Copy link

update-docs bot commented May 17, 2021

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@stotko stotko force-pushed the stotko/cmake-modernization branch 2 times, most recently from 21e4800 to ce24b35 Compare May 19, 2021 17:08
@ssheorey
Copy link
Member

Hey @stotko this is great. I'll complete the review later, but had a few additional suggestions in the mean time:

  • Use cmake-format to consistently format the cmake files.
  • Run cmake-lint to check them for problems.
  • Use CMAKE_MSVC_RUNTIME_LIBRARY to sync STATIC_WINDOWS_RUNTIME of Open3D with 3rd party / external projects. See here for an example. We can simplify some complex code such as this.

Some TODO items for future:

  • Sync security options (e.g.: PIC, stack protection, etc.) from Open3D to 3rd party libraries.
  • Convert current bash script to build CUDA+CPU pybind library to a cmake script so that it works on Windows as well.

@stotko
Copy link
Contributor Author

stotko commented May 25, 2021

Thanks, these are all good points! I think we should address them in future PRs as this one here is just one step in a series of improvements.

* Use `cmake-format` to consistently format the cmake files.

* Run `cmake-lint` to check them for problems.

I manually formatted a subset of files. Doing this systematically, e.g. with a CI job, would be much better. But this is out of the scope of this PR and would add even more noise here.

* Use `CMAKE_MSVC_RUNTIME_LIBRARY` to sync `STATIC_WINDOWS_RUNTIME` of Open3D with 3rd party / external projects. See [here](https://github.com/intel-isl/Open3D/blob/9ad18dc6c3f25b293f085ed20c71db0fd204d5ae/3rdparty/civetweb/civetweb.cmake#L18) for an example. We can simplify some complex code such as [this](https://github.com/intel-isl/Open3D/blob/e77ec7e4eff25203f121315708f1329e35d5f1e2/3rdparty/zeromq/zeromq_build.cmake#L9).

I have not touched the dependency system so far. This deserves some special attention and should be addressed in a separate PR.

* Sync security options (e.g.: PIC, stack protection, etc.) from Open3D to 3rd party libraries.

I already have some local work towards this goal which is build on top of this PR. Extending my existing work to 3rd party libraries should be quite straightforward.

* Convert current `bash` script to build CUDA+CPU pybind library to a cmake script so that it works on Windows as well.

Moving away from bash scripts in general (at least to a certain degree) would be beneficial to simplify the build steps, e.g. in the CI scripts.

@stotko stotko force-pushed the stotko/cmake-modernization branch from ce24b35 to cb55e2d Compare May 25, 2021 08:53
Copy link
Collaborator

@yxlao yxlao left a comment

Choose a reason for hiding this comment

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

Reviewed 54 of 54 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @errissa, @prewettg, @ssheorey, and @stotko)


cpp/open3d/visualization/gui/CMakeLists.txt, line 110 at r1 (raw file):

        OUTPUT "${GUI_MAT_OUTPUT_FILE}"
        COMMAND ${FILAMENT_MATC} ${GUI_MATC_ARGS} -o "${GUI_MAT_OUTPUT_FILE}" "${GUI_MAT_SOURCE_FILE}"
		MAIN_DEPENDENCY ${GUI_MAT_FILE} DEPENDS ${FILAMENT_TARGET}

nit: tab to space


cpp/open3d/visualization/webrtc_server/CMakeLists.txt, line 24 at r1 (raw file):

add_custom_target(copy_html_dir

(this was removed from master)


cpp/pybind/CMakeLists.txt, line 84 at r1 (raw file):

# use the target_compile_definitions approach. Otherwise, use configure_file.
if (BUILD_AZURE_KINECT)
    target_compile_definitions(pybind PRIVATE BUILD_AZURE_KINECT)

Shall all these definitions be set in open3d_set_global_properties and we remove them here?
E.g. BUILD_GUIand WITH_FAISS is already set atopen3d_set_global_properties.

@stotko stotko force-pushed the stotko/cmake-modernization branch from cb55e2d to 93d2599 Compare May 31, 2021 12:40
Copy link
Contributor Author

@stotko stotko left a comment

Choose a reason for hiding this comment

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

Reviewable status: 45 of 55 files reviewed, 2 unresolved discussions (waiting on @errissa, @prewettg, @ssheorey, and @yxlao)


cpp/open3d/visualization/gui/CMakeLists.txt, line 110 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

nit: tab to space

Done.


cpp/open3d/visualization/webrtc_server/CMakeLists.txt, line 24 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

(this was removed from master)

Done.


cpp/pybind/CMakeLists.txt, line 84 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

Shall all these definitions be set in open3d_set_global_properties and we remove them here?
E.g. BUILD_GUIand WITH_FAISS is already set atopen3d_set_global_properties.

Done. There is one intentional leftover in the ml/tensorflow module due to an incompatibility with open3d_set_global_properties. Perhaps, this could be resolved as well (along with other minor cleanups), but I would defer this to a separate PR to avoid blocking this one.

Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

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

Reviewed 40 of 54 files at r1, 10 of 10 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @errissa, @prewettg, @stotko, and @yxlao)


CMakeLists.txt, line 540 at r2 (raw file):

            target_compile_definitions(${target} PRIVATE BUILD_CACHED_CUDA_MANAGER)
        endif()
    endif()

Are the options below sorted? Not sure about the reason for the changes below.

Copy link
Contributor Author

@stotko stotko left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @errissa, @prewettg, @ssheorey, and @yxlao)


CMakeLists.txt, line 540 at r2 (raw file):

Previously, ssheorey wrote…

Are the options below sorted? Not sure about the reason for the changes below.

Yes, I tried to group them similar to how the options are grouped. The order might still not be perfectly consistent, but at least they are now placed together.

Copy link
Collaborator

@yxlao yxlao left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @errissa, @prewettg, @ssheorey, @stotko, and @yxlao)


CMakeLists.txt, line 369 at r2 (raw file):

string(REPLACE ";" "," CUDA_HARDENING_CFLAGS "${HARDENING_CFLAGS}")
string(REPLACE ";" "," CUDA_HARDENING_LDFLAGS "${HARDENING_LDFLAGS}")
add_compile_options(

Does it make sense to move the add_compile_options and add_link_options to open3d_set_global_properties?

@yxlao yxlao merged commit 6107eeb into master Jun 4, 2021
@yxlao yxlao deleted the stotko/cmake-modernization branch June 4, 2021 04:42
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.

3 participants