-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add android library subproject dependencies to the report #134
Add android library subproject dependencies to the report #134
Conversation
…ows/129-update-readme jaredsburrows#129 - update readme to include correct maven repositories
@mkubiczek fantastic - can you please share instructions on how to integrate a pre-release build of this PR into a project to test things out? |
@ened
And last but not least. Please share your feedback ;) |
98960ab
to
cf981d6
Compare
configurationSet.add(configurations.getByName("${flavor.name}Implementation")) | ||
} | ||
} | ||
configurations.find { it.name == "${variant}RuntimeClasspath" }?.also { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much cleaner, we no longer need to check for any "special" flavors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what do you mean by "special" flavors. This is based on my observations that all the dependencies can be found in ${variant}RuntimeClasspath configuration, including subproject dependencies which were missing in the other ones. I've seen there are some tests for flavored builds and since they have passed I assumed we should be fine.
Actually I've seen Google is doing it bit differently in their oss licenses plugin. Instead of specifying configuration that should be taken into account in license generation, they're taking all the configurations and excluding only some of them like the ones that cannot be resolved or tests. Not sure which approach is better though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks. I did not know about that plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that plugin but it has its flaws. Eg it cannot only be applied to a project of type com.android.application
but not when it's com.android.library
which is my use case.
project.dependencies.add(pomConfiguration, gav) | ||
) | ||
set.resolvedConfiguration.lenientConfiguration.allModuleDependencies.forEach { | ||
if (it.configuration == "runtime") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again this was based on my observations that some dependencies like 'runtimeElements' couldn't be resolved due to some ambiguity. But after thinking more about it I'm not convinced to this check. I am not sure if there can be other configurations besides 'runtime' that are fine as well. So I changed the approach based on what google is doing in oss license plugin. Please check the fixup.
Does this PR not affect other "subproject" tests I already wrote in this project? |
c1203ed
to
ba5c774
Compare
I only added extra test for android multimodule project with android module (as there was only one with java-library). I didn't change anything in existing tests so I guess they should be fine. |
} else { | ||
resolvedArtifacts.addAll(resolvedDependency.allModuleArtifacts) | ||
} | ||
} catch (exception: Exception) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename exception
to e
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
resolvedDependencies: Set<ResolvedDependency> | ||
): Set<ResolvedArtifact> { | ||
|
||
val resolvedArtifacts = HashSet<ResolvedArtifact>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this hashSetOf
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@mkubiczek Doesn't this plugin already support subprojects? For example, here is a test I wrote a while ago - https://github.com/jaredsburrows/gradle-license-plugin/blob/master/src/test/groovy/com/jaredsburrows/license/LicensePluginAndroidSpec.groovy#L832. |
It works when the subproject is of type |
@mkubiczek Thanks! Can you fix/update this PR based on my last 2 previous comments and I can merge this? |
ba5c774
to
bdb53bf
Compare
Thanks! |
@jaredsburrows Are you going to publish new version of the plugin in the coming days? |
Sure! |
Done! |
For some reason dependencies that come from android library subproject were not collected. There must be something different between
java-library
andcom.android.library
plugins. I observed though that all the dependencies can be found inxxxRuntimeClasspath
configuration so I used that one instead.This works for my setup and for all the existing tests. But gradle dependency resolution is not my area of expertise so I would appreciate a review of this change.