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

Conversion of build system to CMake #384

Merged
merged 384 commits into from
Sep 20, 2023
Merged

Conversion of build system to CMake #384

merged 384 commits into from
Sep 20, 2023

Conversation

astro-friedel
Copy link
Contributor

@astro-friedel astro-friedel commented May 12, 2023

The build system has been converted from a set of shell scripts to a CMake build system. The code should build on the following systems

  • Fedora 37+
  • Centos Stream9
  • Ubuntu 20.04, 22.04 (18.04 is NOT supported because it is effectively EOL)

The following build system changes were implemented:

  • OS repo based dependencies must be installed by the user prior to running cmake. The only exception to this is OpenCV in CentOS. The OS repo does not provide the full version (the viz module is missing), in this instance CMake will pull the appropriate version from the OpenCV git repo and build and use it.
  • Non-OS repo based dependencies (e.g. DBoW2) are included as external packages in CMake. they are built and installed in the install prefix given for ILLIXR.
  • External plugins (e.g. Kimera_VIO) are also treated as external packages and build in the same fashion as the non_OS based dependencies.
  • All plugins rely on the same versions of OS-based dependencies (e.g. some plugins used a version of Eigen that was 15 years old while others used one that was 5 years old)
  • Both gcc and clang should work for compilation

The following code changes were implemented:

  • Due to changes in some dependency APIs some plugin code was updated to work with the newest versions, some of these changes are backward compatible with older but still supported API versions
  • The "main.cpp" code was updated to be able to take command line arguments for which plugins to load, as well as arguments which were had been specified in environment variables (which are still supported), and a yaml file which contains any of these parameters.

As far as the parameters for the executable go, they will follow this priority:

  1. If the parameter is specified on the command line it will have the highest priority
  2. If the parameter has a value in the yaml file this value will only be used if it is not specified on the command line (second priority)
  3. If the parameter has a value as an environment variable this value will only be used if it is not specified on the command line nor yaml file

Note that not everything may be working completely at this point, but I felt that the build system updates should be tested out as the details are finished up.

Also note that the cmake arguments for which plugins to build can be found here (temporary home)

@charmoniumQ
Copy link
Member

Don't forget to update documentation pages: Building ILLIXR, Modifying a plugin, Writing your own plugin. Write somewhere in the documentation the configuration places and precedence.

Maybe keep some of install_apt_deps.sh, since we are passing off installing OS-repo-based dependencies to the user.

Copy link
Member

@shg8 shg8 left a comment

Choose a reason for hiding this comment

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

For the offload plugin, it would also need gstreamer. Right now this is installed manually by the user. A lot of the distro version of gstreamer is outdated, so, preferably, we would want to pull a branch from their repo here and link statically to it. Offloading currently use gstreamer's base and nvcodec plugins.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@astro-friedel
Copy link
Contributor Author

Regarding leaving parts of the install_apt_deps.sh script, I think it makes a cleaner and more universal to use the cmake and the web page interface that gives you the command line for the packages to install. I have also updated the documentation

@astro-friedel astro-friedel requested a review from shg8 May 18, 2023 20:50
@charmoniumQ
Copy link
Member

Ah, I see the command in the documentation now; that is quite sufficient.

I take back my objection to removing install_apt_deps.sh.

@charmoniumQ
Copy link
Member

What are the pros/cons of vendoring dependencies like sqlite3pp, concurrentqueue, cxxopts vs pulling them in through CMake?

@jianxiapyh
Copy link
Member

jianxiapyh commented Jun 14, 2023

Hi Doug, where is Sophus used? If we need it, I think we need to add libfmt-dev in the package installation as well.

CMakeLists.txt Outdated Show resolved Hide resolved
@astro-friedel
Copy link
Contributor Author

@charmoniumQ

What are the pros/cons of vendoring dependencies like sqlite3pp, concurrentqueue, cxxopts vs pulling them in through CMake?

Using Vendor version:
Pros:

  • these libraries are guaranteed to be stable
  • any dependencies they have will automatically be installed by the vendor package manager
  • this is standard practice in the industry
  • these libraries will be kept up to date and compatible with the rest of the system
  • any plugins that depend on these libraries will be using the same version and avoids conflicts and collisions

Cons:

  • if there is a specific version you need this will not work, but then one would have to think about why a specific version is necessary

Building through CMake
Pros:

  • the specific version is under your control (but, again one would need to think about why a specific version is needed)

Cons:

  • you may need to worry about installing any dependencies of these libraries manually
  • system compatibility is not guaranteed in the long term
  • you will need to manually update the cmake files when a new version is needed
  • Build times will be longer
  • there could be conflicts with vendor installed versions at link/run time
  • not all libraries are easy or friendly to build from source

All in all, I am a big fan of vendor supplied libraries as they make management much easier and stable long term. There are times when building one of these from source is necessary (e.g. OpenCV on CentOS does not provide the viz module, so we build it from source in that instance), but these should be kept to a minimum.

@astro-friedel
Copy link
Contributor Author

@jianxiapyh

Hi Doug, where is Sophus used? If we need it, I think we need to add libfmt-dev in the package installation as well.

Sophus is used by the ORB_SLAM plugin. libfmt is not a required dependency for Sophus, but if we want to use it I can certainly add it to the list.

@charmoniumQ
Copy link
Member

What are the pros/cons of vendoring dependencies like sqlite3pp, concurrentqueue, cxxopts vs pulling them in through CMake?
All in all, I am a big fan of vendor supplied libraries

I meant vendoring as copying the source code of third-party library into our source-tree (ref).

If this is your preference, Concurrentqueue and cxxopts do have Ubunut packages we could switch to (I don't know about Fedora or CentOS).

Sqlite3pp has no Ubuntu package, so it can either remain vendored in our repository or we can pull down a copy from the internet.

@wsherman64
Copy link
Contributor

wsherman64 commented Jun 14, 2023 via email

@qinjunj qinjunj added this to the 3.2 milestone Jun 15, 2023
@astro-friedel
Copy link
Contributor Author

@charmoniumQ

What are the pros/cons of vendoring dependencies like sqlite3pp, concurrentqueue, cxxopts vs pulling them in through CMake?
All in all, I am a big fan of vendor supplied libraries

I meant vendoring as copying the source code of third-party library into our source-tree (ref).

If this is your preference, Concurrentqueue and cxxopts do have Ubunut packages we could switch to (I don't know about Fedora or CentOS).

Sqlite3pp has no Ubuntu package, so it can either remain vendored in our repository or we can pull down a copy from the internet.

All of these are very small projects (some even just header only) so I could go either way (vendoring or using the OS repo) for these. For Sqlite3pp vendoring would be the only option. Ideally we would make these submodules, rather than having the code directly in our repo.

@astro-friedel
Copy link
Contributor Author

@wsherman64

A note on the OpenCV need of the "vis module". If this is what I'm remembering, the requirement of the OpenCV "vis" module is actually a problem for users of VTK who want a modern version of VTK. My recollection is that the OpenCV vis module wants version 6.x of VTK, but VTK is now on the 9.x version, so OpenCV is massively out of date in that regard. It'd be better if we could avoid requiring the ancient VTK. I'm not even sure there's a great use of OpenCV's vis module in ILLIXR. Bill

For Ubuntu 18.04 this is true, but it is now end of life. Ubuntu 20.04 natively runs VTK 7.1, and Ubuntu 22.04 runs VTK 9.1, both with compatible OpenCV versions. I do not know off the top of my head what versions run in Fedora or CentOS (I suspect that Fedora is at least as recent as Ubuntu).

If someone wants to have VTK 9 and a compatible OpenCV on an older system they will be responsible for installing it themselves. It is easy enough to do from their git repos.

Copy link
Contributor

@qinjunj qinjunj left a comment

Choose a reason for hiding this comment

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

Hi Doug, the file build/_deps/kimera_vio/tmp/Kimera_VIO-gitclone.cmake has empty paths for some git commands. I'm sending the file to your email.

@jianxiapyh
Copy link
Member

  1. I am also running into the empty paths issue qinjunj mentioned above.
  2. It seems the current build assumes that the user has OpenCV prebuilt in their system. However, since ILLIXR still runs a specific version of OpenCV (3.4.6) should we install OpenCV in the build script?

@astro-friedel astro-friedel self-assigned this Jul 6, 2023
@astro-friedel astro-friedel added documentation Improvements or additions to documentation feature New feature or request labels Jul 6, 2023
@astro-friedel
Copy link
Contributor Author

@qinjunj

Hi Doug, the file build/_deps/kimera_vio/tmp/Kimera_VIO-gitclone.cmake has empty paths for some git commands. I'm sending the file to your email.

This is now fixed

@astro-friedel astro-friedel dismissed shg8’s stale review September 18, 2023 22:38

the issues has been resolved

@astro-friedel astro-friedel mentioned this pull request Sep 19, 2023
@qinjunj
Copy link
Contributor

qinjunj commented Sep 20, 2023

Can we add default data path and demo_data path to the yaml files in profiles? For e.g., upzip the downloaded data.zip and set data_path: data/mav0, demo_data: ../demo_data

cmake/GetMonado_vk.cmake Outdated Show resolved Hide resolved
cmake/GetMonado_gl.cmake Outdated Show resolved Hide resolved
@astro-friedel
Copy link
Contributor Author

@qinjunj You can specify data and demo_data entries for each profile. Keep in mind that if the data needs to be downloaded it must be a url, not a local file path.

@qinjunj
Copy link
Contributor

qinjunj commented Sep 20, 2023

@qinjunj You can specify data and demo_data entries for each profile. Keep in mind that if the data needs to be downloaded it must be a url, not a local file path.

Yeah I can update that myself. Was only wondering if we should set a default value so that the user will have something to run out of box. After all we have demo_data already there and data.zip downloaded with cmake. But if you prefer not and have that specified clearly in the doc, I'm okay with that. Also for the ?? part in the doc, you can put --enable_offload, save rendered frames, enable_alignment, apply estimated alignment parameters to poses in pose_lookup, enable_pre_sleep, activate sleeping at application start for e.g. attaching gdb.

docs/docs/plugin_README/offload_vio.md Show resolved Hide resolved
docs/docs/virtualization.md Show resolved Hide resolved
docker/README.md Show resolved Hide resolved
@astro-friedel astro-friedel merged commit 493ab80 into master Sep 20, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature New feature or request infrastructure Build and deployment infrastructure plumbing Something that connects/interfaces/plugs into many parts of ILLIXR.
Projects
None yet
10 participants