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 request] Support encapsulation of the dependencies' headers #4038

Closed
DoDoENT opened this issue Dec 2, 2018 · 4 comments
Closed
Assignees
Milestone

Comments

@DoDoENT
Copy link
Contributor

DoDoENT commented Dec 2, 2018

Let's consider the following scenario:

  • package Aaa contains some utilities
  • package Bbb uses package Aaa, but it's public API headers do not depend in any way on headers from Aaa
  • package Ccc uses package Bbb, however, a developer of Ccc package accidentally included headers from Aaa and used functions from it, without adding Aaa to the requires attribute of the Ccc's conanfile.py. This worked correctly for the developer of the Ccc package but will cause major problems to the users of the Ccc package, as explained below.
  • a downstream user (in provided example, a Ddd package) uses only Ccc package, but at some point decides to override Aaa to v2.0.0. Since Bbb depends on Aaa, Conan rebuilds the Bbb but it does not rebuild Ccc, as Conan does not know that Ccc uses Aaa internally. This causes a linker error to the developer of the Ddd package (or further downstream if Ddd is another static lib) in the best scenario, or even worse, it causes the misbehaving application that is almost impossible to debug.

The solution to this problem would be to Conan not exposing Aaa's headers to the Ccc package unless there is a direct dependency on it.

I tried setting Aaa as private dependency of the Bbb in
this example, however, although this resulted with the desired behaviour in Ccc package, it caused a linker error in Ddd package, as Aaa's binaries were not linked in.

The correct behaviour would be if Conan generated the build system in a way that when some package is a direct dependency, the build system's target generated for that package would contain information about include paths, preprocessor definitions, compile flags, link flags and link libraries, but when some package is an indirect (transitive) dependency, the build system's target generated for that package would contain only link flags and link libraries, but no compile information, such as preprocessor definitions, compile flags and include paths.

In this case, when compiling Ccc that includes Aaa's headers, you will get a compile error, unless you state that Ccc depends on Aaa in its requires list. However, when using Ccc from Ddd or further downstream, you will not get a linker error, as Aaa's binaries will get linked into the final binary.

But there is a catch...

However, sometimes some libraries have public headers that depend on public headers of another library. In that case, you want the current behaviour, except that breaking change of that library should trigger a rebuild of all libraries that see its headers. For example, if Bbb's public headers include public headers from Aaa, then overriding Aaa from Ddd should also trigger the rebuild of Ccc, as it uses Bbb's public header that depends on Aaa.

To make this mess less complicated and much easier to understand, I suggest introducing additional parameters to the requires attribute. Currently, it supports only two parameters: override and private and I suggest adding api and implementation parameters (the idea actually comes from gradle - I really suggest reading this link).

If Bbb states that it requires Aaa for its implementation (which I argue should be the default when nothing is specified), something like this:

class Bbb(ConanFile):
    requires = ('Aaa/1.0.0@user/testing', 'implementation'),

Then, Ccc will get a compile error if it tries to include headers from Aaa (unless it adds direct dependency on Aaa), but Ddd will still link correctly, unlike in the case when Bbb's dependency on Aaa is private (private is most similar to gradle's compileOnly).

However, if Bbb states that it requires Aaa also for its API, something like this:

class Bbb(ConanFile):
    requires = ('Aaa/1.0.0@user/testing', 'api'),

Then, Ccc will be able to include headers, compile flags and preprocessor definitions from Aaa, even though it does not have a direct dependency on Aaa. However, the Conan will then know that if Ddd or someone further downstream overrides Aaa with a breaking version, that Ccc also needs to be rebuilt, i.e. it's package ID will have to depend on Aaa.

Current behaviour treats package ID as in implementation mode, and include path propagation as in api mode, thus making a very hard time for code reviewers to manually detect cases when a developer included a header that is either not from a direct dependency or includes a header from a transitive dependency.

I argue that this feature is a must for Conan v2.0, but let's discuss whether it may be possible to add this even to v1.x. Stating that either implementation or api mode is the default will definitely break existing packages: api would cause different package IDs to be generated for current dependency chains, which would cause massive rebuilds of everything, which is very bad and implementation would break packages that were broken in the first place because they used headers from transitive dependencies.
To make things backward compatible I suggest adding a legacy parameter to the requires attribute which would be the default and would have the exact same behaviour as Conan currently has, except that it will print out a warning that it should not be used due to problems mentioned here (similar as you have done with transition of default_options from being a tuple to being a dictionary). Then, in v2.0 you can simply remove the legacy parameter and set the implementation to be the default.

@jgsogo
Copy link
Contributor

jgsogo commented Dec 6, 2018

(IMHO) I think that something like this should be taken into account for v2.0 (and backported to v1.0 if it is possible without breaking anything), and all of this mixed with the DLL Hell too... 😅

@jgsogo
Copy link
Contributor

jgsogo commented Mar 28, 2019

I'm adding this issue to the milestone 2.0, we won't have time to think about it for a 1.x release, but we will want to consider it for 2.0 for sure.

@DoDoENT
Copy link
Contributor Author

DoDoENT commented Nov 4, 2020

Here is my workaround for this issue. It treats all existing dependencies in implementation mode and does not support api mode, as this requires changes in conan graph model, while my workaround happens in CMake after including conanbuildinfo.cmake:

    # extract all direct dependencies from conaninfo.txt
    file( STRINGS "${CMAKE_BINARY_DIR}/conaninfo.txt" conan_info )
    set( direct_conan_dependencies )
    set( in_requires_block OFF )
    foreach( line ${conan_info} )
        if ( ${line} STREQUAL "[requires]" )
            set( in_requires_block ON )
        elseif ( in_requires_block )
            if ( ${line} MATCHES "\\[[a-z]+\\]" )
                break()
            else()
                string( FIND "${line}" "/" pkg_separator_pos )
                string( SUBSTRING "${line}" 0 ${pkg_separator_pos} pkg_name )
                string( STRIP "${pkg_name}" pkg_name )
                list( APPEND direct_conan_dependencies "CONAN_PKG::${pkg_name}" )
            endif()
        endif()
    endforeach()

    message( STATUS "Direct dependencies: ${direct_conan_dependencies}" )

    foreach( conan_target ${CONAN_TARGETS} )  # CONAN_TARGETS is defined in conanbuildinfo.cmake, generated by conan
        list( FIND direct_conan_dependencies ${conan_target} direct_deps_pos )
        if ( ${direct_deps_pos} EQUAL -1 )  # not a direct dependency
            # remove all include directories of transitive dependency
            set_property( TARGET ${conan_target} PROPERTY INTERFACE_INCLUDE_DIRECTORIES "" )
        endif()
    endforeach()

Basically, it parses conaninfo.txt to determine which dependencies are direct and removes the INTERFACE_INCLUDE_DIRECTORIES from all Conan targets that are not direct dependencies.

This then forces the developer to define all their dependencies in their conanfile. This gets annoying if public headers in Bbb include public headers in Aaa as it forces all users of Bbb to also add a dependency to Aaa (such a thing requires changes to conan graph model which I don't know about enough to make changes 😞 ). This is annoying, but less annoying than debugging ODR violations and random bugs caused by conan not rebuilding Ccc after Ddd overrides the Aaa in above example.

NOTE: Adding self.info.requires['Aaa'].semver_mode() in package_id method of recipe of Bbb will not trigger rebuild of Ccc if Ddd overrides Aaa. As far as I know, there is no way for Bbb to communicate downstream that it's public headers are also public headers of Aaa, or is there?

@memsharded
Copy link
Member

Implemented and released in 2.0-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

4 participants