khrykin / StrategrDesktop

No-Fuss Time Management App
https://khrykin.github.io/strategr/
GNU General Public License v3.0
311 stars 34 forks source link

some small fixes #5

Closed borco closed 4 years ago

khrykin commented 4 years ago

Thank you for the input! There are some things to discuss before we could merge:

borco commented 4 years ago
khrykin commented 4 years ago
borco commented 4 years ago

According to https://github.com/nlohmann/json/blob/develop/README.md, you have to use something like this:

# CMakeLists.txt
find_package(nlohmann_json 3.2.0 REQUIRED)
...
add_library(foo ...)
...
target_link_libraries(foo PRIVATE nlohmann_json::nlohmann_json)

Without it, the code didn't compile on my machine -- how would you preffer the CMakeLists.txt to handle it?

khrykin commented 4 years ago

Right now I have a symlink to nlohmann folder (the one with json.hpp file) in /usr/local/include on my Mac, which makes it automatically accessible for the compiler, without any cmake intervention. If it doesn't compile this way, you could check that /usr/local/include is set as an include location for your compiler, which it should.

Again, I'm thinking of moving all dependencies except boost and Qt right into the project tree to reduce the hassle for the new contributors. In this case we could make folders like third-party/lib and third-party/include, then in CmakeLists.txt we would just include_directories(third-party/include) and set(CMAKE_LIBRARY_PATH third-party/lib).

borco commented 4 years ago

I guess there is no perfect solution to this, but including other projects into yours, especially if you plan to split their sources will become a maintenance problem in the future, if you want to upgrade the dependencies.

In my opinion, adding them as git submodules under third-party works better. Especially because you can then create a custom CMakeLists.txt in third-party that replaces the ones provided by those third-library packages and this way provide a better/customized integration with your app.

This is what I currently do in my Qt apps. For example, in this older app of mine (more or less...): https://github.com/borco/tutcatalog/tree/master/src/3rd-party

khrykin commented 4 years ago

Submodules would be ok for header-only libs, but I was thinking of including pre-built binaries for all supported platforms, so people wouldn't have to compile specific versions of dependencies themselves. If we want to upgrade - recompile new versions, swap binaries and headers, that's it?

borco commented 4 years ago

I am very much against including binary libs that can be built from sources into git and transform git into some kind of CI artifact placeholder. Better configure the repo to use AppVeyor and Travis if you want some precompiled stuff. That’s my opinion, at least.

khrykin commented 4 years ago

Anyway, at the moment there's only utf8proc binary dependency besides boost and Qt, so let's not overthink this right now - I would just leave it as it's now on the master.