cycfi / elements

Elements C++ GUI library
http://cycfi.github.io/elements/
3.13k stars 237 forks source link

tidying up json and infra dependencies #106

Closed Xeverous closed 4 years ago

Xeverous commented 4 years ago

Right now elements depends on infra and json, which itself depends on infra.

I have a problem with filesystem because certain range of GCC and Clang versions require an extra flag to link to it. I would like to make in an explicit CMake option because as I stated in https://github.com/cycfi/infra/issues/4 there is no good approach to it so IMO the most flexible solution would be to let user decide. Then, having such option I can downstream it to any dependants (including elements).

There is however a different problem: elements and json does not link to infra, not in the CMake way. Currently the both CMake recipes are just providing include paths to them, which is wrong because they should link to their targets instead and expect complete definitions there. Otherwise we duplicate recipes and leak target definition to dependant projects.

My current plan:

Then we can have a straight dependency chain (infra => json => elements) and targets depending on elements can get downstreated infra settings (eg filesystem linker options).

Any objections?

djowel commented 4 years ago

I don't think we need to complicate matters. The simpler solution is to use ghc::filesystem on such compilers. I already do that in MacOS: https://github.com/cycfi/infra/blob/master/include/infra/filesystem.hpp#L9-L11

djowel commented 4 years ago

There is however a different problem: elements and json does not link to infra, not in the CMake way.

There's a reason why I did it that way: non-hierarchical and multiple dependencies. This is one thing that cmake handles very poorly. Cmake forces you to think in clean hierarchies. IMO, we shouldn't need to link to infra. It should remain header only, after all.

Xeverous commented 4 years ago

I don't think we need to complicate matters.

This is not a complication. Currently elements and json do not depend on infra (as seen by CMake). Whatever I configure there it is not forwarded because CMake sees elements and json as single targets. If something is to be configured (eg which filesystem implementation to use, whether to add any linker flags), the dependands must be aware of the configuration.

This is a simplification. Currently include paths are duplicated in multiple project recipes because elements hardcodes them to its submodules instead of pulling them from targets. From elements point of view, the recipes in json and infra do not exist (they are not actually used in any way).

There's a reason why I did it that way: non-hierarchical and multiple dependencies. This is one thing that cmake handles very poorly. Cmake forces you to think in clean hierarchies.

CMake handles this very well. If A depends on B, whatever B depends on (eg C) will be given at build/usage to A. If A gets a missing include/compile/linker command to C, that's a bug in the recipe of B.

IMO, we shouldn't need to link to infra. It should remain header only, after all.

That's actually good that you wrote this statement because it reveals that you do not understand what (indeed misleadingly/unintuitively named) target_link_libraries is actually doing in modern CMake.

What you think of is the legacy version, that is, target_link_libraries(foo bar...). That's basically -lbar flag.

The new version (target_link_libraries(foo [PUBLIC | PRIVATE | INTERFACE] bar...)) behaves much different, it should be named more like target_depends_on. Modern CMake separates 2 core concerns with 3 keywords:

The command then downstreams all build and usage requirements to targets that need it. This includes include paths, compiler features, linker flags, target sources and more. Basially every target_* requirement is forwarded to descendant projects.

I do not want to change infra to require compilation. There is no need to. I only want descendant targets to express in modern CMake way that they actually depend on it.

Note: the same principle applies to boost. By target_link_libraries(foo PUBLIC Boost::boost) the target foo expresses that it depends on (not links in the compiler sense) header-only libraries of boost. No actual linker commands are added to foo, only include paths.

I do not want to paste how the full algorithm in works in CMake, but I want to just express that every possible combination of dependency chain with any libraries (header-only, compileable, imported, object) with any requirements (build and/or usage) is working. Example: If C requires compilation and B, a header only library depends on it and A, a compilable library depends on B, then builds of A will get the link flag for C, just because B told to do even though B is not actually build.

djowel commented 4 years ago

What is this link flag:

I have a problem with filesystem because certain range of GCC and Clang versions require an extra flag to link to it.

And what are the implications of this link flag to a header only library?

  • remove elements/infra submodule - it is already a submodule of json on which elements depends

This is what I mean by what CMake handles poorly. The dependency is this:

    Infra   
   |     | 
   |   JSON
   |     |
   Elements    

And not this:

   Infra   
     |     
    JSON
     |    
  Elements  
djowel commented 4 years ago

CMake recipes are just providing include paths to them, which is wrong because they should link to their targets instead and expect complete definitions there.

"Wrong" and "should" again seems to assume following a certain set of rules. And again, I am not a rule follower. Why it is wrong is unclear to me, based on your explanation.

Otherwise we duplicate recipes and leak target definition to dependant projects.

I think you lost me here. Leaking what? The include file path? And how is there duplication?

djowel commented 4 years ago

Anyway, having said all that..., I think I am fine with this as long as infra remains header only and there will be no awkward link (in the normal sense of the word) dependencies.

One thing though: is removing infra from elements as a submodule really necessary because of a cmake shortcoming? The duplicated submodule is IMO a reflection of the real dependency as I outlined in the graph above. But, nevertheless, not a big deal.

So, well... Go ahead, proceed.

At any rate. I'd still appreciate some replies answers to my questions above, though.

Xeverous commented 4 years ago

What is this link flag:

posted in https://github.com/cycfi/infra/issues/4

And what are the implications of this link flag to a header only library?

The flag will be a usage requirement. Since header-only libraries (interface targets) are not build, the flag will not apply to infra itself, as linker flags are only used by compilable targets. But because it is a usage requirement, anyting that uses (depends on) infra will get the flag. If that thing is another header-only library, the same will happen. The flag will only be added to a target that finally requires building - then the usage requirement of infra will be added to build and usage requirements of such compilable target.

Note: this is one of many complex cases of the dependency algorithm in CMake that forwards/adds build/usage requirements through interface/object/library/executable target dependency chain.

"Wrong" and "should" again seems to assume following a certain set of rules. And again, I am not a rule follower. Why it is wrong is unclear to me, based on your explanation.

So let me reiterate on modern CMake target-based approach though an example.

There is a library C.

There is a library B

There is a library A that is using B.

Now, with modern CMake:

I think you lost me here. Leaking what? The include file path? And how is there duplication?

Yes, this is violating the bolded point. Elements specifies something (infra public includes) that should be a part of its dependency definition. That's not elemens job, it should be done by infra. The only elements jobs are to define itself and what it depends on.

Xeverous commented 4 years ago

One thing though: is removing infra from elements as a submodule really necessary because of a cmake shortcoming? The duplicated submodule is IMO a reflection of the real dependency as I outlined in the graph above. But, nevertheless, not a big deal.

I get your dependency graph - elements uses both libraries directly. I know that such dependency graphs are handled well with find_package (it's just "found" multiple times) but not sure what happens if multiple paths on the dependency tree include the same target definition - it's like the diamond inheritance problem. I guess CMake will probably error about duplicate definition, but that could be easily handled with if(NOT TARGET infra) to include infra subdirectory conditionally.

Xeverous commented 4 years ago

CMake already has a very simple solution: https://cmake.org/cmake/help/v3.17/command/include_guard.html. Unfortunately it is only for include command, not add_subdirectory

https://stackoverflow.com/questions/42978414/how-to-handle-a-transitive-dependency-conflict-using-git-submodules-and-cmake/ SO basically says the same: use ifs to avoid adding the target multiple times.

djowel commented 4 years ago

One thing though: is removing infra from elements as a submodule really necessary because of a cmake shortcoming? The duplicated submodule is IMO a reflection of the real dependency as I outlined in the graph above. But, nevertheless, not a big deal.

I get your dependency graph - elements uses both libraries directly. I know that such dependency graphs are handled well with find_package (it's just "found" multiple times) but not sure what happens if multiple paths on the dependency tree include the same target definition - it's like the diamond inheritance problem. I guess CMake will probably error about duplicate definition, but that could be easily handled with if(NOT TARGET infra) to include infra subdirectory conditionally.

I know I've been there before and I got some funny results with cmake. There's nothing wrong with diamonds! It's not inheritance. It happens all the time with C/C++ libraries. In c++ header-only libs, it's simply handled using include guards. Very simple, because it is simple. All that matters is the #include , no more, no less. No complications. No nonsense. You want it private? Sure, include it in the cpp file only, and use forward declarations. Again simple, no nonsense.

Oh and BTW, at one point I experimented on #pragma once too. It has the same problem when you have two same include files in separate locations, like the dependency graph I have above.

I'm probably OK with your "solution", but I find it kinda awkward and kinda hackish workaround over cmake's limitations, because although it can work in this situation, there might be other situations that it might not. That is my worry. I use infra not only in elements, but also in Q and QPlug. My fear is that I might be approving on something that will break the other dependencies.

djowel commented 4 years ago

I think you lost me here. Leaking what? The include file path? And how is there duplication?

Yes, this is violating the bolded point. Elements specifies something (infra public includes) that should be a part of its dependency definition. That's not elemens job, it should be done by infra. The only elements jobs are to define itself and what it depends on.

I think you are still overly complicating matters to suit cmake. Infra is a public header only dependency of elements and json. Including infra in a header file that depends on elements OR json will leak the interface no matter what you do. It's not a cmake issue. It is a c++ issue. There are remedies: include in cpp files, or use modules.

djowel commented 4 years ago

So the way I understand it is that you do all these elaborate dance in order to get the linker flags in? But what I am saying is that you do not need the linker flags at all if you just use ghc::filesystem on problematic compilers. ghc::filesystem does not require such linker flag nonsense.

I don't know, to me the solution is very obvious and simple. I understand how you want to make the cmake gods happy, but... :-)

Xeverous commented 4 years ago

So the way I understand it is that you do all these elaborate dance in order to get the linker flags in?

yes :)

But what I am saying is that you do not need the linker flags at all if you just use ghc::filesystem on problematic compilers. ghc::filesystem does not require such linker flag nonsense.

Right, but then I need to forward a flag to use ghc implementation.

So no matter what I choose I need to forward some usage requirements to descendant targets.

Xeverous commented 4 years ago

you do not need the linker flags at all if you just use ghc::filesystem

"just using" actually has no possibility now because GCC 8 falls into the std ifdefs: it has __cplusplus >= 201703L and __has_include(<filesystem>).

I would like to add explicit CMake options to force use of specific implementation, and use the current ifdefs is not specified.

djowel commented 4 years ago

you do not need the linker flags at all if you just use ghc::filesystem

"just using" actually has no possibility now because GCC 8 falls into the std ifdefs: it has __cplusplus >= 201703L and __has_include(<filesystem>).

Well, then fix it by detecting __clang__. There should be a way to do this in the c++ world.

djowel commented 4 years ago

Which compilers and which versions exactly are having problems with filesystem? The link (*) you gave is using quite an old compiler that requires <experimental/filesystem>. For such old compilers, you should just use ghc::filesystem.

(* This link: https://stackoverflow.com/questions/53807011/undefined-reference-error-with-new-filesystem-library-and-clang7)

Xeverous commented 4 years ago

GCC 8.4.0

It has the <filesystem> but requires linker flag.

djowel commented 4 years ago

How about just detecting that using these?

__GNUC__
__GNUC_MINOR__
__GNUC_PATCHLEVEL__

(https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html)

and switching to ghc::filesystem like I did with apple clang and msvc?

https://github.com/cycfi/infra/blob/master/include/infra/filesystem.hpp#L9-L11

Xeverous commented 4 years ago

Why do you so heavily insist on detecting this? I would like to have explicit options to force use of specific implementation. I'm already using std::filesystem elsewhere in my project.

djowel commented 4 years ago

Why do you so heavily insist on detecting this? I would like to have explicit options to force use of specific implementation. I'm already using std::filesystem elsewhere in my project.

Because it is the simpler and safer solution. It does not require hacking cmake by changing the dependency structure, which may break other projects that also depend on infra and elements at the same time. The simplest solution is always the best solution.

Xeverous commented 4 years ago

Have a look at https://github.com/Xeverous/infra/commit/6d40c67bf00d7211a20c4d2d656bc8b5bb389fc9

This preserves all of existing behavior by default.

djowel commented 4 years ago

Have a look at Xeverous/infra@6d40c67

This preserves all of existing behavior by default.

Looks good to me!

I'm fine as long as it allows this non-hierarchical dependency structure and nothing will break in the end when I add infra as a dependency on other libraries which also depend on elements and json:

         +<--- json <---+<-----------------+
         |              |                  |
infra <--+--------------+--- elements <----+
         |                                 |
         +<--- qlib <----------------------+
         |                                 |
         +<--- ----------------------------+-- qplug

Forcing cmake to artificially use a hierarchical dependency structure is what got me worried.

djowel commented 4 years ago

This is interesting!

if (TARGET infra)
    return()
endif()
Xeverous commented 4 years ago

Forcing cmake to artificially use a hierarchical dependency structure is what got me worried.

The linked SO question had some discussion related on the topic, the ultimate thing is that when you have diamond dependencies you need to handle it explicitly because the same dependency can not exist in multiple versions. AFAIK in Python package requirements, if there is a diamond dependency tree and the duplicated package has multiple, incompatible versions the project is rejected. It is not considered to be a build/package system shortcoming, it's more about implementation: I don't know of any progrmming language that would make it possible to run/link the same dependency multiple times with different versions. In theory it is possible but in practice no one does that.

In case of infra or json there is no such problem as we always point to the newest master (so any diamond dependency has copies of exactly the same version of infra). Having a CMake "target add-subdir guard" is the cleanest solution.

djowel commented 4 years ago

Thanks for the detailed explanation! Let's do it then.

Xeverous commented 4 years ago

https://github.com/cycfi/infra/pull/7 is ready

djowel commented 4 years ago

cycfi/infra#7 is ready

Great news! merged.

Xeverous commented 4 years ago

Now https://github.com/cycfi/json/pull/5

After it, elements can get rid of all manually added paths.

Xeverous commented 4 years ago

Now #108 and the whole issue will be complete.