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

[FEATURE REQ]: Better dependency handling #6

Open
okawo80085 opened this issue Jan 16, 2022 · 3 comments
Open

[FEATURE REQ]: Better dependency handling #6

okawo80085 opened this issue Jan 16, 2022 · 3 comments
Assignees
Labels
back-burner Temporarily won't be worked on driver Driver related enhancement New feature or request

Comments

@okawo80085
Copy link
Member

Is your feature request related to a problem? Please describe.
Our driver starts to grow in the number of external libraries it depends on, right now all that is handled in it's CMakeLists file and each library needs to be added by hand, each of them has different paths, weird source trees (looking at you openvr), etc.

Describe the solution you'd like
Since we have forks of all of them external libs we're using, we should format them all to follow a single convention for source trees and build instructions, so that our driver's CMakeLists doesn't grow larger then the driver itself...

Describe alternatives you've considered
I considered other build systems like bazel and a few python based ones, but the amount of effort needed to transition to those is too much imo.

Additional context
If we don't come up with some standard to follow early on, it will really bite us in the ass later on.

@okawo80085 okawo80085 added the enhancement New feature or request label Jan 16, 2022
@okawo80085
Copy link
Member Author

Almost done with this one actually, it was easier than i expected.

@okawo80085 okawo80085 self-assigned this Jan 26, 2022
@okawo80085 okawo80085 pinned this issue Jan 27, 2022
@okawo80085
Copy link
Member Author

So the solution i found, that is more or less painless, is: all dependencies must use CMake and be setup for tool chains, while also having a repo tree like this:

# lets say the name of the dep is "example"
# so the repo structure will look like this
example/
├── CMakeLists.txt
├── LICENSE
├── README.md
├── src
│   ├── example.h
│   └── # and other src files
└── test
    ├── CMakeLists.txt
    └── # test src files

The main dep build recipe should not set up any global variables or declarations, only target specific commands.
It needs to have tests, but they need to be off by default (i'm not building dep tests in production, just no, our driver takes ages to compile as is).
The main include header for the dep needs to be located (and named) in <dep_repo_root>/src/<dep_name>.h and the dep needs to declare only a single target named <dep_name>.

All of that to achieve this behavior when using the dep.

CMakeLists.txt in the main project:

...

# setup toolchain
add_subdirectory("<dep_name>")
set(<dep_name>_INCLUDE_DIR "<dep_repo_root>/src/")

...

# add dep to include path
target_include_directories(main_target
    ...
    ${<dep_name>_INCLUDE_DIR}
    ...
)

...

# link dep target
target_link_libraries(main_target
    <dep_name>
    ...
    ${CMAKE_DL_LIBS}
)

...

@okawo80085
Copy link
Member Author

It'll take some time to move all of our deps to this mental model (cough cough openvr cough cough), but it's worth it and most of our deps are already pretty much setup for this, requiring only a few tweaks (like disabling test builds or changing some compile options).

@okawo80085 okawo80085 added the back-burner Temporarily won't be worked on label Mar 27, 2022
@okawo80085 okawo80085 unpinned this issue Mar 27, 2022
@okawo80085 okawo80085 added the driver Driver related label May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-burner Temporarily won't be worked on driver Driver related enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant