gradle-native icon indicating copy to clipboard operation
gradle-native copied to clipboard

C++ transitive implementation dependencies should not leak into downstream consumers

Open big-guy opened this issue 6 years ago • 10 comments

Given:

project(":app") {
    apply plugin: 'cpp-application'

    application {
        dependencies {
            implementation project(":library")
        }
    }
}

project(":library") {
    apply plugin: 'cpp-library'

    library {
        dependencies {
            implementation project(":unwanted")            
        }
    }
}

project(":unwanted") {
    apply plugin: 'cpp-library'
}

When compiling app, I currently do not see unwanted's headers in the include path (good), but I do still see libunwanted when linking. For the most part, this prevents accidental use of libunwanted APIs, but if app defines the symbol from libunwanted:

extern int foo_from_unwanted ();

It's possible to use this API from unwanted and have no link/runtime failures. If library then changes to remove its implementation dependency on unwanted, app will suddenly start failing.

This also happens with statically linked libraries. Static libraries make this a little trickier because they're like extra object files that may require extra dependencies.

big-guy avatar Jul 16 '18 22:07 big-guy

@big-guy Thank you for logging this issue. I have confirmed with my builds that, using transitive = false or not, my list of compile-time dependencies is correct but my link-time dependencies is a long list, even with all dependencies being linked in as implementation

this is the precise issue my team is running into with our Gradle builds. Do you have an estimate for when this issue might be addressed?

AnnDinKC avatar Jul 18 '18 18:07 AnnDinKC

Hey y'all! This is valuable to me given the (unfortunate) number of externs across our codebase.

Would a fix for this look something like https://github.com/gradle/gradle/compare/master...b-john:transitive_lib_fix ?

        Configuration nativeLink = configurations.create(names.withPrefix("nativeLink"));
        // ...
        nativeLink.setTransitive(false);
        nativeLink.extendsFrom(getImplementationDependencies());

Where we would just set "Transitive" for the "getImplementationDependencies" configuration to false?

It looks like the transitive flag should be flexed based off api or implementation dependency but not seeing anywhere across the code base that leverages that. Thoughts? Appreciate the help, thanks!

b-john avatar Aug 09 '18 21:08 b-john

Looks like with the above proposal, the ordering of the libraries/dependencies becomes necessary though. Before, Gradle was properly ordering my library dependencies correctly but with the above change its on the user to place them properly.

b-john avatar Aug 10 '18 18:08 b-john

This issue is a bit tricky because transitive dependency could be valid in one case.

When compiling a shared library variant, unwanted would be linked inside library thus doesn't and should be exposed to app.

When compiling a static library variant, exposing the static library of unwanted to app could be a reasonable approach if library use unwanted implementation as opposed to just the headers. The idea is both variants would require the same dependency but just declared differently. The best would be for Gradle to decide when it makes sense to expose the dependency transitively if the implementation is actually used. If you want to build library and substitute the implementation of unwanted for another library, you should use the dependency substitution rules offered by Gradle as opposed to declaring a different dependency on app.

This is just how I feel it should work to avoid trivial mistake and manual handling of such scenario by curating your dependencies. Gradle should work with you not the opposite.

lacasseio avatar Aug 20 '18 09:08 lacasseio

Thanks Daniel-- The thought is if the libraries are static it would be desired behavior to keep as-is/automatically include them in the consuming app?

  • app A depends on static B which depends on static C, the link line for A should include both B & C?

Our problem is better stated by the Fedora community: https://fedoraproject.org/wiki/UnderstandingDSOLinkChange . While it is more convenient for A to get C by pulling in B, if A directly depended on C then it puts project B in a bad spot where it can never remove its dependency on C without breaking downstream consumers.

  • Here's a test app if it helps clarify: https://github.com/b-john/gradle-transitive-static

Thoughts? I agree the ideal solution would be a smarter system that could detect if a dependency was needed directly or transitively (just for statics), but to my knowledge there isn't an elegant solution to do that for C++/GNU which is why Fedora and others require us to list out every direct dependency of our included non-shared object files.

  • Aka, the difference between a static library (archive of non-shared object files (not ELF file)) vs object file within the project is minimal.

Greatly appreciate your time, thanks Daniel!

b-john avatar Aug 20 '18 15:08 b-john

Sorry, I would further complicate this by your point from 2 years ago: https://github.com/gradle/gradle/issues/801#issuecomment-258258496

If we have bad library design, the static library might have some object files which depend on dependency D... some that depend on dependency E... etc. The consuming application may only need a subset of object files within the static library and therefore only need dependency D or E to compile and may not want to pull in extra dependencies.

C++ nightmares. Thanks again!

b-john avatar Aug 20 '18 15:08 b-john

Hey y'all, here's the changes I made to make this work in case it helps: https://github.com/gradle/gradle/compare/master...b-john:ordered_no_trans_deps

@lacasseio, interested in your thoughts. I would not expect this to be merged as-is given it it fairly poor (c++ dev writing java), targets just Linux/GCC, and there's better ways to do chunks of it. I apologize for the length & complexity here but these features were a must-have for me. I greatly appreciate your time, thoughts, and ideas. Thanks!


Problem: Linker line is including all transitive dependencies

Why this is a problem

See https://github.com/gradle/gradle-native/issues/759#issuecomment-414347803. This was exemplified farther when linking against a shared library which linked against a static library; All consumers of the shared library automatically linked in the internal static library which has static singletons/vars in it that weren't meant to be in all binaries.

What was done to fix this

Set the "nativeLink" to false for the transitive configuration. https://github.com/b-john/gradle/blob/c2c97a33ef4d7d0cfc430ae669d8fb10a749fcab/subprojects/language-native/src/main/java/org/gradle/language/cpp/internal/DefaultCppBinary.java#L100

What problems did fixing it cause

Problem 1: Library order listed in the dependencies now mattered. Gradle was handling library order properly for us with transitive but lost that with no transitive.

Problem 2: CPP Applications/Tests failed to link due to needing transitive shared libraries on the library path or specified with "-rpath".

Problem: Library order must specified correctly by user in build.gradle

Why this is a problem

If static library A depends on static library B which depends on static library C, the dependencies had to be listed in an A,B,C order in the build.gradle file or would throw unresolved symbol for GCC. The order matters for GCC: https://stackoverflow.com/questions/45135/why-does-the-order-in-which-libraries-are-linked-sometimes-cause-errors-in-gcc/409470#409470

Gradle could already figure out the order (was doing so previously) and it's painful on the user to have to know which order to place the libraries. It would also be more non-passive changes to make users update their build.gradle if order mattered.

What was done to fix this

I am confident y'all would know a better way (maybe mess with the ConfigurationResolver class directly); But I introduced a new configuration "nativeDirectLink" which did non-transitive (unordered but direct dependencies) while keeping "nativeLink" transitive (ordered but includes superset (transitive) dependencies). https://github.com/b-john/gradle/blob/c2c97a33ef4d7d0cfc430ae669d8fb10a749fcab/subprojects/language-native/src/main/java/org/gradle/language/cpp/internal/DefaultCppBinary.java#L94

I was then able to "logically AND" the two sets together to get an ordered, direct-only dependency list: https://github.com/b-john/gradle/blob/c2c97a33ef4d7d0cfc430ae669d8fb10a749fcab/subprojects/platform-native/src/main/java/org/gradle/nativeplatform/tasks/AbstractLinkTask.java#L260

Problem: GCC requires all shared library dependencies to be on the library path

Why this is a problem

GCC requires all shared libraries dependencies (transitively) be reachable at link time by going through this order listed under "-rpath-link": https://linux.die.net/man/1/ld We want our libraries to be in configurable places at runtime (leverage library path) per this issue: https://github.com/gradle/gradle-native/issues/142 and therefore wanted option number 5.

What was done to fix this

Keep all dependencies (transitive or not) on the library path: https://github.com/b-john/gradle/blob/c2c97a33ef4d7d0cfc430ae669d8fb10a749fcab/subprojects/platform-native/src/main/java/org/gradle/nativeplatform/tasks/AbstractLinkTask.java#L256

And then hackily pass that to the CommandLineToolInvocationContext as an environment variable, setting LD_LIBRARY_PATH for all linker runs: https://github.com/b-john/gradle/blob/c2c97a33ef4d7d0cfc430ae669d8fb10a749fcab/subprojects/platform-native/src/main/java/org/gradle/nativeplatform/toolchain/internal/AbstractCompiler.java#L86

b-john avatar Aug 30 '18 18:08 b-john

Thanks @b-john for this detailed comment. I was hoping to have a look last week but didn't happen. The priority of this is low at the moment so I will try my best to come back on this as quickly as possible.

lacasseio avatar Sep 10 '18 14:09 lacasseio

Any updates?

hansaya avatar Sep 18 '19 21:09 hansaya

Unfortunately, there hasn't been any progress on this. If you feel particularly motivated by this issue, we can discuss a plan and split it into small steps that you can help on.

lacasseio avatar Sep 19 '19 14:09 lacasseio