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

fix: Include dependency substitutions in dependency extractors #31

Closed
wants to merge 3 commits into from

Conversation

rainecp
Copy link

@rainecp rainecp commented Aug 8, 2024

This addresses an issue with the affected-paths calculator where projects depending on included builds were not being returned when changes occurred in those included builds. This is becauseConfiguration.allDependencies used in dependency extractor was including artifacts from the included builds rather than the project dependency substitutions.

Details:

This updates DependencyExtractors.kt to include project dependencies from resolved artifacts.

  • The new method checks if the configuration can be resolved (e.g. compileOnly configuration cannot be resolved).
  • It retrieves dependencies from the resolution result and differentiates between direct and transitive dependencies.
  • It filters project dependencies and converts their paths so it will supported in the affected-paths logic.

Testing:

Published a snapshot version and used that against a gradle project with included builds. Verified that the new affected-paths accounts for projects dependent on included builds.

@pablobaxter pablobaxter self-assigned this Aug 8, 2024
Copy link
Collaborator

@pablobaxter pablobaxter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@inez @tmellor-block We're going to need to add some tests to handle the dependency substitution use case this PR is fixing. Maybe a quick follow?

@pablobaxter
Copy link
Collaborator

So looking further at what is being resolved here, this is actually solving an issue with dependency substitution of an included build. I believe this to be a good fix to the issue, although the concern I have is that it would increase the time spent by the Gradle daemon configuring, since now it is resolving dependencies. I wonder if having a flag for configuration resolution should be considered?

Copy link
Collaborator

@pablobaxter pablobaxter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Remove RELEASE_SIGNING_ENABLED=false

@rainecp rainecp changed the title fix: Handle includeBuild projects in dependency extractors fix: Include dependency substitutions in dependency extractors Aug 9, 2024
Copy link
Collaborator

@inez inez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure to address @pablobaxter's comments. Thx!

@rainecp
Copy link
Author

rainecp commented Aug 12, 2024

Please make sure to address @pablobaxter's comments. Thx!

Thanks for reviewing this. Yes, that has been addressed here 2f95be9. I also discussed with Pablo this implementation last week and resolved #31 (comment).

@pablobaxter
Copy link
Collaborator

Pulled in all commits and work into this PR: #32.

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

Successfully merging this pull request may close these issues.

3 participants