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

Cleanup dependencies #1315

Merged
merged 8 commits into from
Jul 4, 2024
Merged

Cleanup dependencies #1315

merged 8 commits into from
Jul 4, 2024

Conversation

SimonMarquis
Copy link
Contributor

This is a followup cleanup of #1163 that was partially addressed by #1140.

  • Remove unused projects.core.testing dependencies (or replace with direct dependencies).
  • Introduce androidx.compose.ui.test bundle.
  • Remove NiaTestRunner from the default config, forcing consumers to depend on it, even when not used.

Here is the current diff on the generated instrumented test APKs (assembleDemoDebugAndroidTest):

find . -name "*.apk" -exec du --human-readable --summarize '{}' \;
- 1.6M    ./app/build/outputs/apk/androidTest/demo/debug/app-demo-debug-androidTest.apk
+ 1.2M    ./app/build/outputs/apk/androidTest/demo/debug/app-demo-debug-androidTest.apk
- 19M     ./core/database/build/outputs/apk/androidTest/demo/debug/database-demo-debug-androidTest.apk
+ 3.9M    ./core/database/build/outputs/apk/androidTest/demo/debug/database-demo-debug-androidTest.apk
- 20M     ./core/designsystem/build/outputs/apk/androidTest/demo/debug/designsystem-demo-debug-androidTest.apk
+ 18M     ./core/designsystem/build/outputs/apk/androidTest/demo/debug/designsystem-demo-debug-androidTest.apk
  20M     ./core/ui/build/outputs/apk/androidTest/demo/debug/ui-demo-debug-androidTest.apk
  20M     ./feature/bookmarks/build/outputs/apk/androidTest/demo/debug/bookmarks-demo-debug-androidTest.apk
  20M     ./feature/foryou/build/outputs/apk/androidTest/demo/debug/foryou-demo-debug-androidTest.apk
  20M     ./feature/interests/build/outputs/apk/androidTest/demo/debug/interests-demo-debug-androidTest.apk
  20M     ./feature/search/build/outputs/apk/androidTest/demo/debug/search-demo-debug-androidTest.apk
  21M     ./feature/settings/build/outputs/apk/androidTest/demo/debug/settings-demo-debug-androidTest.apk
  20M     ./feature/topic/build/outputs/apk/androidTest/demo/debug/topic-demo-debug-androidTest.apk
- 20M     ./sync/work/build/outputs/apk/androidTest/demo/debug/work-demo-debug-androidTest.apk
+ 11M     ./sync/work/build/outputs/apk/androidTest/demo/debug/work-demo-debug-androidTest.apk

This is a followup cleanup of android#1163 that was partially addressed by android#1140.

- Remove unused `projects.core.testing` dependencies (or replace with direct dependencies).
- Introduce `androidx.compose.ui.test` bundle.
- Remove `NiaTestRunner` from the default config, forcing consumers to depend on it, even when not used.
Using latest graphviz 10.0.1
@dturner
Copy link
Collaborator

dturner commented Mar 26, 2024

Introduce androidx.compose.ui.test bundle.

What's the purpose of introducing the bundle?

The change was an internal version name contained in a comment.
@SimonMarquis
Copy link
Contributor Author

The main purpose of this bundle is to avoid the need to specify both androidx.compose.ui:ui-test-junit4 and androidx.compose.ui:ui-test-manifest dependencies everytime.
They almost always need to come together, and if you forgot the test-manifest, your code will compile, but the test will fail at runtime with an error that is not very easy to debug (and the issue mentionned in the stackstrace is not really explicit...):

Stacktrace

java.lang.RuntimeException: Unable to resolve activity for Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] cmp=com.google.samples.apps.nowinandroid.core.designsystem.test/androidx.activity.ComponentActivity } -- see https://github.com/robolectric/robolectric/pull/4736 for details
	at org.robolectric.android.internal.RoboMonitoringInstrumentation.startActivitySyncInternal(RoboMonitoringInstrumentation.java:105)
	at org.robolectric.android.internal.LocalActivityInvoker.startActivity(LocalActivityInvoker.java:36)
	at org.robolectric.android.internal.LocalActivityInvoker.startActivity(LocalActivityInvoker.java:41)
	at androidx.test.core.app.ActivityScenario.launchInternal(ActivityScenario.java:362)
	at androidx.test.core.app.ActivityScenario.launch(ActivityScenario.java:202)

@dturner
Copy link
Collaborator

dturner commented May 13, 2024

Sorry @SimonMarquis, do you mind resolving the conflicts here please?

@SimonMarquis
Copy link
Contributor Author

@dturner

@SimonMarquis
Copy link
Contributor Author

@dturner this is once again ready to me reviewed/merged :)

@dturner
Copy link
Collaborator

dturner commented Jul 4, 2024

Thanks for doing this.

It'd be good if we could generate dependency graphs other configurations as well (such as for test and androidTest). I've filed jraska/modules-graph-assert#261 to request this.

@dturner dturner merged commit c0f9660 into android:main Jul 4, 2024
4 checks passed
@SimonMarquis SimonMarquis deleted the fix/1161-2 branch July 5, 2024 18:31
@SimonMarquis
Copy link
Contributor Author

SimonMarquis commented Jul 7, 2024

@dturner what would be your thoughts about integrating this graph generation process directly into this project as an extra plugin in build-logic?
This should be fairly simple, and we could adapt it to fit our needs. Like, removing the red path, filtering test/androidTest configurations, etc.
This could also be a good opportunity to get mermaid graphs directly in the markdown files since they are automatically rendered in GitHub (and maybe no longer export SVGs).
Another advantage would be to run the tasks in all modules in parallel, 'cause right now we need to execute them one after the other, and the script takes a long time to go through all the modules.

@SimonMarquis
Copy link
Contributor Author

SimonMarquis commented Jul 7, 2024

Here is a quick example of how it can look like:

  • dotted/normal arrows for implementation/api configuration
  • grouping nodes by package
  • specific color for module types
%%{
  init: {
    'theme': 'dark'
  }
}%%

graph LR
  subgraph :core
    :core:ui["ui"]:::android-library
    :core:designsystem["designsystem"]:::android-library
    :core:data["data"]:::android-library
    :core:analytics["analytics"]:::android-library
    :core:model["model"]:::jvm-library
    :core:common["common"]:::android-library
    :core:database["database"]:::android-library
    :core:datastore["datastore"]:::android-library
    :core:network["network"]:::android-library
    :core:notifications["notifications"]:::android-library
  end
  subgraph :feature
    :feature:topic["topic"]:::android-library-selected
  end

  :feature:topic -.-> :core:ui
  :feature:topic -.-> :core:designsystem
  :feature:topic -.-> :core:data

  :core:ui ---> :core:analytics
  :core:ui ---> :core:designsystem
  :core:ui ---> :core:model

  :core:data ---> :core:common
  :core:data ---> :core:database
  :core:data ---> :core:datastore
  :core:data ---> :core:network
  :core:data -.-> :core:analytics
  :core:data -.-> :core:notifications

  :core:database ---> :core:model

  :core:network ---> :core:common
  :core:network ---> :core:model

  :core:notifications ---> :core:model
  :core:notifications -.-> :core:common

classDef android-library-selected fill:#3BD482,stroke:#000,stroke-width:2px,color:#fff;
classDef android-library fill:#3BD482,stroke:#fff,stroke-width:2px,color:#fff;
classDef jvm-library fill:#8150FF,stroke:#fff,stroke-width:2px,color:#fff;

Loading

This looks better than the current Graphviz dot render:

dep_graph_feature_topic

But it could still be improved to look like the mermaid version:

graphviz

@dturner
Copy link
Collaborator

dturner commented Jul 8, 2024

My primary concern is around maintenance - if the build-config plugin approach is less maintenance than the current, somewhat hacky bash script then I'm all for it. The Mermaid graphs definitely look better, and the fact that they can be directly added to a markdown file, rather than needing a separate SVG is also a positive (more info).

@SimonMarquis
Copy link
Contributor Author

I feel like the local plugin approach will remain untouched once it is finished. I know that Gradle APIs changes a lot but, if we use recent APIs we should not really be impacted by this. And I think using external Gradle plugins would end up in the same situation if they were to ban some APIs. I'll push my work-in-progress after some cleanup in a draft PR, but I feel like we should first discuss what we want from these graphs.
Opening a discussion could be usefull if we want to gather more feedbacks.
For example you mentioned being able to show other Gradle configurations like test/androidTest. Do you think it would make sense to show them in the same graph? I personally feel like it would make things potentially harder to reason about.

@SimonMarquis
Copy link
Contributor Author

I've opened this discussions:

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