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.

Sign up for GitHub

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

Jump to bottom

Cache dex syntethic context in dex builder #20411

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Cache dex syntethic context in dex builder #20411

wants to merge 2 commits into from

Conversation

mauriciogg
Copy link
Contributor

There is a data race when using workers when there are multiple libraries that include the same class. This is more likely to happen when building multiple android_binary targets in a single invocation. When a cached entry is found, only the dex content is used and the sythetic info is ignored. In those instances, the dex archive will be incorrectly created and might lead to failures during dex merging

Fixes: bazelbuild/rules_android#300

@mgalindo-sc
Cache dex syntethic context in dex builder
e837ba2 
There is a data race when using workers when there are multiple
libraries that include the same class. This is more likely to happen
when building multiple android_binary targets in a single invocation.
When a cached entry is found, only the dex content is used and the
sythetic info is ignored. In those instances, the dex archive will
be incorrectly created and might lead to failures during dex merging
@github-actions github-actions bot added team-Android Issues for Android team awaiting-review PR is awaiting review from an assigned reviewer labels Dec 1, 2023
@mauriciogg mauriciogg mentioned this pull request Dec 1, 2023
Open
@ahumesky
Copy link
Contributor

ahumesky commented Dec 5, 2023

I imported this and started running our internal tests against it. Also, is there a test we can add to src/test/java/com/google/devtools/build/android/r8 for this case?

@ahumesky ahumesky self-assigned this Dec 5, 2023
@mauriciogg
Copy link
Contributor Author

I imported this and started running our internal tests against it. Also, is there a test we can add to src/test/java/com/google/devtools/build/android/r8 for this case?

Added a new test. I had to expose processRequest to the test. Otherwise I'd have to start a new process to in order to go through the workers flow.

@valeraz valeraz mentioned this pull request Dec 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkoroste nkoroste nkoroste approved these changes

@ahumesky ahumesky Awaiting requested review from ahumesky ahumesky is a code owner

@ted-xie ted-xie Awaiting requested review from ted-xie ted-xie is a code owner

Assignees

@ahumesky ahumesky

Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Android Issues for Android team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Conflicting version of R8/D8 in Dexer/Desugar classpath
4 participants
@mauriciogg @ahumesky @nkoroste @mgalindo-sc