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

build(ci): ensure testlib is built #17531

Closed
wants to merge 1 commit into from

Conversation

tanishy7777
Copy link

Added a dependency on the assemblePlayDebugAndroidTest task from AnkiDroid's androidTest in build.gradle.

Purpose / Description

Fixes the issue #17520 by adding a dependency on the assemblePlayDebugAndroidTest task from AnkiDroid's androidTest in build.gradle.

Fixes

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

added a dependency on the assemblePlayDebugAndroidTest task from AnkiDroid's androidTest in build.gradle.
Copy link

welcome bot commented Dec 1, 2024

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Hey there 👋 welcome to AnkiDroid

One subtle suggestion here but otherwise, this seems like it should work!

You erased the testing section of the PR template though so I am not sure how you proved to yourself it worked?

Please re-insert that section, then show the results of a ./gradlew androidTest -v (I think -v prints out the executed tasks...) before and after the change to see if the new task dependency is definitely missing before and definitely there after in the list of executed tasks

@@ -281,6 +281,11 @@ tasks.register('copyTestLibIntoAndroidTest', Copy) {
tasks.named('preBuild').configure { dependsOn('copyTestLibIntoAndroidTest') }
tasks.named('runKtlintCheckOverAndroidTestSourceSet').configure { mustRunAfter('copyTestLibIntoAndroidTest') }

// to run manually: `./gradlew :testlib:assemblePlayDebugAndroidTest`
tasks.named("assemblePlayDebugAndroidTest") {
Copy link
Member

Choose a reason for hiding this comment

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

The other task dependencies use .configure for a reason: it is done that way in order to work well with Gradle's ability to defer configuration, which can speed compilation for jobs that don't use some tasks and thus don't need to configure them

This was suggested as something to change via gradle lint rules in one of the recent gradle updates

68b1f68

Suggested change
tasks.named("assemblePlayDebugAndroidTest") {
tasks.named("assemblePlayDebugAndroidTest").configure {

@mikehardy
Copy link
Member

Actually, it looks like it didn't compile at all

  • Where:
    Build file '/home/runner/work/Anki-Android/Anki-Android/AnkiDroid/build.gradle' line: 285

Did you run this prior to submitting it?

@david-allison david-allison changed the title Fixes #17520 build(ci): ensure testlib is built Dec 1, 2024
@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Dec 3, 2024
@david-allison david-allison marked this pull request as draft December 6, 2024 00:03
@MritunjayTiwari14
Copy link

MritunjayTiwari14 commented Dec 8, 2024

@mikehardy Shall i work on this issue?
hamcrest and hamcrest-core both dependencies are included in the gradle build, both contain overlapping classes such as org.hamcrest.BaseMatcher which is leading to duplicate class error. Also these modules are transient dependency so we have to find which dependency is including either of them and then excluding one of them so that gradle does not throw duplicate class errors

@mikehardy
Copy link
Member

@tanishy7777 do you intend to get this working locally and respond to the comment?

@MritunjayTiwari14 I'm not sure what you are talking about. I'm not sure what hamcrest etc have to do with it, I don't see any errors related to that.

@MritunjayTiwari14
Copy link

MritunjayTiwari14 commented Dec 9, 2024

@mikehardy Alright, just saw the other PR where David had already fixed the Dependency issue. Sorry for disturbing.

@david-allison
Copy link
Member

david-allison commented Dec 9, 2024

To note: this issue (#17520) is a follow-on from that PR

@MritunjayTiwari14
Copy link

MritunjayTiwari14 commented Dec 9, 2024

@david-allison just want to confirm that is Gradle still throwing those duplicate class errors while trying to compile, because in #17520 which is still unresolved the compilation still throws duplicate class errors.

@david-allison
Copy link
Member

david-allison commented Dec 9, 2024

Looks fine to me. The point of this issue is to make sure these errors don't reappear

➜  Anki-Android git:(ktlint-dep-2) ✗ git checkout main && ./gradlew :testlib:assemblePlayDebugAndroidTest
Switched to branch 'main'
Your branch is ahead of 'origin/main' by 72 commits.
  (use "git push" to publish your local commits)
Starting a Gradle Daemon (subsequent builds will be faster)

> Task :AnkiDroid:compilePlayDebugJavaWithJavac
Note: /Users/davidallison/StudioProjects/Anki-Android/AnkiDroid/build/generated/aidl_source_output_dir/playDebug/out/android/content/pm/IPackageStatsObserver.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
[Incubating] Problems report is available at: file:///Users/davidallison/StudioProjects/Anki-Android/build/reports/problems/problems-report.html

BUILD SUCCESSFUL in 58s
105 actionable tasks: 14 executed, 91 up-to-date

Copy link
Contributor

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Dec 23, 2024
@github-actions github-actions bot closed this Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author New contributor Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: ensure testlib is built
5 participants