Skip to content

refactor: refactor build to cmake conventions#64

Open
cpehle wants to merge 1 commit intomainfrom
refactor-build
Open

refactor: refactor build to cmake conventions#64
cpehle wants to merge 1 commit intomainfrom
refactor-build

Conversation

@cpehle
Copy link
Copy Markdown
Contributor

@cpehle cpehle commented Mar 29, 2023

The proposed directory structure more closely represent what you would expect for a CMake project.

@cpehle cpehle requested a review from Jegp March 29, 2023 10:31
@Jegp
Copy link
Copy Markdown
Contributor

Jegp commented Mar 30, 2023

Nice idea :-)

Do you have a reference to how a project is expected to be structured? Just so I can better understand the setting.

One thing we may want to consider before going forward with something like this would be the structure of camera drivers (#66). If, for instance, we branch out the camera input code to a separate repo, we'd need to move the headers somewhere, too.

@cpehle
Copy link
Copy Markdown
Contributor Author

cpehle commented Mar 30, 2023

Do you have a reference to how a project is expected to be structured? Just so I can better understand the setting.

Well not using relative include paths is pretty much a universally accepted convention. It leads to non-portable code (e.g. https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4464?view=msvc-170). The split into headers and source files is also pretty universally done, although there is more disagreement there. It would also be in line with conventions to have one source directory and only expose public headers in the include directory. But in any case it isn't a "good" idea to use relative include paths.

One thing we may want to consider before going forward with something like this would be the structure of camera drivers (#66). If, for instance, we branch out the camera input code to a separate repo, we'd need to move the headers somewhere, too.

I think this can be managed by setting up sensible and perhaps lower granualarity CMake targets and carefully managing optional dependencies.

@Jegp
Copy link
Copy Markdown
Contributor

Jegp commented Aug 20, 2023

Thanks again for the efforts here :-)

Did you have a chance to look at the changes following #89? They reorganized the content quite a bit. Would you be interested in adapting your changes to the main branch? I believe your suggestion is a valuable addition.

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.

2 participants