pitchfork icon indicating copy to clipboard operation
pitchfork copied to clipboard

Feedback on project layout guidelines

Open TheLartians opened this issue 5 years ago • 6 comments
trafficstars

Hey thanks for writing this list of guidelines for consistent structuring of C++ projects, which are strongly needed imo! They have recently been brought to my attention through an issue in my modern C++ starter template and I wanted to share my thoughts on the guidelines here as well.

  • Inconsistent naming conventions: In the proposition some directory names are singular (include, build, external), some are plural (tests, examples, extras), while others are abbreviated (src, libs, docs). IMO directory naming should be consistent so there is no confusion how to properly name directories.

  • Embedding external projects IMO external projects should never be embedded in a project, as this will create problems for library users downstream. E.g. if they wish to use two libraries A and B which both use C as an embedded library, their project will include C twice, thus breaking the ODR. Projects should always use a dependency management tool that fetches dependencies on demand and can be easily overwritten downstream (such as CPM.cmake) to allow users to resolve any issues.

  • No additional top-level directories the recommendations state that "other directories should not be present in the root directory, except for what is required by other tooling". I think this restriction is very project-dependent and reasonable exceptions should be allowed. E.g. I often use a script directory for non-development related scripts (e.g. files to run certain workflows with the compiled binary). My template even has a top-level standalone target executable that allows to run the library as a command-line utility. This is because the development of the standalone is so strongly coupled to the library that it makes no sense to create an additional project for it.

  • Derived targets I'm not sure if this is in the scope of the guidelines but I'd love to see a paragraph on derived targets (e.g. the tests). In many C++ projects (including my own) I've seen the habbit of adding derived projects from the library's build system (e.g. by including it in from the main CMakeLists.txt). IMO this should be strongly discouraged, as it inverts the dependency tree (the tests should actually depend on the library) and make the library harder to include from other projects, as we usually don't want the tests to be included.

  • Submodules If you are using submodules in a C / C++ project that cannot be modelled as an external dependency or project, it is a strong sign that the library itself needs refactoring. This is why I would avoid mentioning them at all in project guidelines.

I hope that the feedback makes sense and would love watch this project evolve!

TheLartians avatar Apr 20 '20 08:04 TheLartians

  • Embedding external projects IMO external projects should never be embedded in a project, as this will create problems for library users downstream. [...]
  • Submodules If you are using submodules in a C / C++ project that cannot be modelled as an external dependency or project, it is a strong sign that the library itself needs refactoring.

Just saying: not every project is a library. I've been working on quite a few applications, where putting the libraries we dependet on in a git submodule below "libs" ("external" in pitchfork terms) was the most efficient, simplest and flexible way to handle dependencies. Admittedly, some of them needed to be crosscompiled for an embedded target.

MikeGitb avatar Apr 20 '20 09:04 MikeGitb

Good point, I'm mostly thinking about library projects, as they are the ones that should be encouraged most to follow the guidelines. Standalone application "endpoints" should always be allowed more flexibility in their choice of tools.

However, even for non-library projects I would recommend adding a few lines in CMake over submodules as its much easier to use and less error-prone. For example, adding

CPMAddPackage(
  NAME Catch2
  GITHUB_REPOSITORY catchorg/Catch2
  VERSION 2.5.0
)

target_link_libraries(app Catch2) 

to the CMakeLists will always fetch and add catch2 in the specified version whereas submodules can easily get out-of-sync and introduce other hard to track issues. And once dependencies start adding submodules of their own the project will become a huge mess to maintain correctly, which is why I would discourage any submodule usage in project guidelines.

TheLartians avatar Apr 20 '20 10:04 TheLartians

whereas submodules can easily get out-of-sync

How? Git submodules record a specific commit. Also, personally I don't like it if build/configure scripts start to download depencies - reagardless of whether its a application or library - but thats just me.

And once dependencies start adding submodules of their own the project will become a huge mess to maintain correctly,

True, but those are libraries, where I completely agree that those should not manage their dependencies via submodules for that very reason.

Again, I don't disagree with what you wrote. I just wanted to point out that the pitchfork proposal is for both apps and libraries and your comments seemed to only target rules for libraries.

MikeGitb avatar Apr 20 '20 10:04 MikeGitb

How? Git submodules record a specific commit.

From personal experience, either when the user forgot to update the submodules after pulling or switching branches (e.g. calling git submodule update --init --recursive), or when the user accidentally modifies code in the submodule and forgets to commit and push the submodule changes, unaware that the code does not reside in the main project. IMO these issues are quite common but maybe that's just me. ¯\_(ツ)_/¯

Also, personally I don't like it if build/configure scripts start to download depencies - reagardless of whether its a application or library - but thats just me.

Which is fair, however I'm afraid we should get used to it as it's how most modern build systems for other languages (npm, cargo, gradle, etc) handle dependencies nowadays. Also, dependencies usually are cached, so that no download will occur most of the time.

TheLartians avatar Apr 20 '20 11:04 TheLartians

How? Git submodules record a specific commit.

Something as simple as a force push could make the commit the submodule is based on invalid.

ArashPartow avatar May 03 '20 23:05 ArashPartow

From personal experience, either when the user forgot to update the submodules after pulling or switching branches (e.g. calling git submodule update --init --recursive), or when the user accidentally modifies code in the submodule and forgets to commit and push the submodule changes, unaware that the code does not reside in the main project. IMO these issues are quite common but maybe that's just me. ¯_(ツ)_/¯

If I had a penny for everytime this heppened. I would have a lot of pennies :D

BTW, cmake now has its own FetchContent which is pretty nice.

h00shi avatar Jun 14 '20 18:06 h00shi