Skip to content

Fix small false-positive for ViewBinding #711

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

Merged

Conversation

tasomaniac
Copy link
Contributor

@tasomaniac tasomaniac commented Jun 6, 2022

I recently opened #710 about suppressing view binding check. I had to suppress since there was a false positive. I looked into our project more and figured out what was wrong.

This is coming from developer-menu module where we have different layout for debug and release versions. In case you want to have different layout in different build-types, you have to separate them. But then you can use them in main source set when accessing. ModuleCheck does not account for this valid (but uncommon) case.

This PR fixes the issue in most simplest way with a test case. Let me know if this makes sense. I'm not sure if I missed anything. If you have concerns about the change, I hope the test case will be helpful.

@tasomaniac tasomaniac requested a review from RBusarow as a code owner June 6, 2022 21:38
@tasomaniac tasomaniac marked this pull request as draft June 6, 2022 21:39
@tasomaniac tasomaniac changed the title Reproduce false-positive result for ViewBinding Fix small false-positive for ViewBinding Jun 6, 2022
@tasomaniac tasomaniac marked this pull request as ready for review June 6, 2022 21:49
@tasomaniac
Copy link
Contributor Author

tasomaniac commented Jun 7, 2022

There is insufficient memory for the Java Runtime Environment to continue.
Native memory allocation (mmap) failed to map 117440512 bytes for Failed to commit area from 0x0000000090000000 to 0x0000000097000000 of length 117440512.

@RBusarow
Copy link
Member

RBusarow commented Jun 7, 2022

There is insufficient memory for the Java Runtime Environment to continue.
Native memory allocation (mmap) failed to map 117440512 bytes for Failed to commit area from 0x0000000090000000 to 0x0000000097000000 of length 117440512.

Yeah...

I moved the test matrix out of GitHub Actions and into dynamic test factories. It's great locally, but the memory issues with TestKit get multiplied and that's too much for the free Windows GHA runner to handle. I'm hoping to move those tests over to an EC2 instance soon. In the meantime, the tests usually pass on the first retry. :)

Copy link
Member

@RBusarow RBusarow left a comment

Choose a reason for hiding this comment

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

Thanks for this! I understand the problem.

Ultimately, this problem affects all Android resources. I'll add something later to handle those cases, but this fix is perfectly fine and it unblocks you, so LGTM. :)

@RBusarow RBusarow merged commit aead3c5 into rickbusarow:main Jun 7, 2022
RBusarow added a commit that referenced this pull request Jun 8, 2022
* main:
  add Algolia search to the website
  consume self again with version 0.12.3
  release docs for 0.12.3
  fix suppressing findings within the AGP DSL fixes #710
  Fix small false-positive for ViewBinding (#711)
  Update dependency com.vanniktech:gradle-maven-publish-plugin to v0.20.0

# Conflicts:
#	modulecheck-gradle/platforms/internal-android/build.gradle.kts
#	modulecheck-gradle/platforms/internal-jvm/build.gradle.kts
#	modulecheck-gradle/plugin/build.gradle.kts
#	modulecheck-parsing/psi/src/test/kotlin/modulecheck/parsing/psi/KotlinAndroidGradleParserTest.kt
#	modulecheck-rule/impl-factory/build.gradle.kts
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