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

Misc dependency rake improvements #444

Merged
merged 7 commits into from
Jul 6, 2023
Merged

Misc dependency rake improvements #444

merged 7 commits into from
Jul 6, 2023

Conversation

ZacSweers
Copy link
Collaborator

See individual commits!

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

  1. In the Usage section, it would be helpful to explain what the command does and what the expected output is. This will provide more context for users who are not familiar with the tool.

  2. In the Implementation section, it would be helpful to provide a brief overview of the DependencyRake.kt file and its purpose in the project.

  3. In the Usage section, it would be helpful to provide an example of how to run the aggregateMissingIdentifiers task and explain what it does. This will help users understand how to use this task and what its purpose is.

  4. In the DependencyRake.kt file, the missingIdentifiersFile property should be annotated with @OutputFile instead of @InputFile, since it is an output file.

  5. In the DependencyRake.kt file, the missingIdentifiers parameter in the rakeProject function should be passed by reference (MutableSet<String>) instead of by value (Set<String>). This will allow the function to modify the set and add missing identifiers to it.

  6. In the DependencyRake.kt file, the mapIdentifier function should return a nullable Coordinates object instead of a non-null Coordinates object. This will allow the function to return null when an unknown identifier is encountered, instead of throwing an error.

  7. In the DependencyRake.kt file, the toDependencyNotation function should take an additional parameter missingIdentifiers: MutableSet<String>. This parameter should be passed to the mapIdentifier function to add missing identifiers to the set.

  8. In the DependencyRake.kt file, the toDependencyString function should take an additional parameter missingIdentifiers: MutableSet<String>. This parameter should be passed to the toDependencyNotation function to add missing identifiers to the set.

  9. In the DependencyRake.kt file, the aggregate function of the MissingIdentifiersAggregatorTask should use sortedSetOf() instead of toSortedSet() to ensure consistent ordering of the aggregated missing identifiers.

  10. In the DependencyRake.kt file, the MissingIdentifiersAggregatorTask should be registered only if the slackProperties.enableAnalysisPlugin is true and the project is not the platform project. This will prevent the task from being registered multiple times and avoid unnecessary configuration.

  11. In the StandardProjectConfigurations.kt file, the rakeDependencies task should be registered only if the slackProperties.enableAnalysisPlugin is true and the project is not the platform project. This will prevent the task from being registered multiple times and avoid unnecessary configuration.

  12. In the StandardProjectConfigurations.kt file, the aggregator task should be configured to use the rakeDependencies task's missingIdentifiersFile as input. This will ensure that the aggregator task runs after the rakeDependencies task and collects the missing identifiers from all subprojects.

Copy link

@autonomousapps autonomousapps left a comment

Choose a reason for hiding this comment

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

Should I make a comment about the lack of tests? :D

To run dependency rake in a project, use the below command

```bash
$ ./gradlew rakeDependencies -Pslack.gradle.config.enableAnalysisPlugin=true --no-configuration-cache

Choose a reason for hiding this comment

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

--no-configuration-cache so sad, I should fix that.


Sometimes dependency rake will try to replace identifiers with ones that are not present in any available
version catalogs. Sometimes this is acceptable, but often times it can result in "missing" dependencies from
the build after it runs. To help fix these, DR will write all missing identifiers out to a build output file.

Choose a reason for hiding this comment

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

I'm curious about this, can you elaborate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically we have a lot of transitive deps that DAGP offers as "add" advice but don't exist in our version catalogs, so this is intended to help make setting those up easier

@@ -129,7 +143,7 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr
advices
.filter { it.isRemove() }
.filterNot { it.coordinates.identifier in MANAGED_DEPENDENCIES }
.associateBy { it.toDependencyString("UNUSED") }
.associateBy { it.toDependencyString("UNUSED", missingIdentifiers) }

Choose a reason for hiding this comment

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

Also just curious about these magic strings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just debug logging!

@@ -110,16 +117,23 @@ constructor(objects: ObjectFactory, providers: ProviderFactory) : AbstractPostPr
val redundantPlugins = projectAdvice.pluginAdvice
val advices: Set<Advice> = projectAdvice.dependencyAdvice
val buildFile = buildFileProperty.asFile.get()
val missingIdentifiers = mutableSetOf<String>()

Choose a reason for hiding this comment

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

missing identifiers just means missing from your version catalog, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup!

@ZacSweers ZacSweers merged commit b491c1d into main Jul 6, 2023
3 checks passed
@ZacSweers ZacSweers deleted the z/rakeFixed branch July 6, 2023 05:27
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.

2 participants