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

Allow optimization and obfuscation of the SDK by reducing proguard rules #2031

Merged
merged 7 commits into from
May 25, 2022

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented May 10, 2022

Closes #1976

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2022

Codecov Report

❗ No coverage uploaded for pull request base (6.x.x@134eb16). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             6.x.x    #2031   +/-   ##
========================================
  Coverage         ?   81.06%           
  Complexity       ?     3211           
========================================
  Files            ?      229           
  Lines            ?    11804           
  Branches         ?     1568           
========================================
  Hits             ?     9569           
  Misses           ?     1650           
  Partials         ?      585           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 134eb16...426d9be. Read the comment docs.

@marandaneto marandaneto changed the title Remove not needed proguard rules Allow optimization and obfuscation of the SDK by reducing proguard rules May 10, 2022
@marandaneto
Copy link
Contributor Author

@romtsn what's about -renamesourcefileattribute SourceFile? it's been removed but it was in the official docs.

@romtsn romtsn marked this pull request as ready for review May 24, 2022 13:57
@romtsn romtsn self-requested a review as a code owner May 24, 2022 13:57
@romtsn
Copy link
Member

romtsn commented May 24, 2022

@romtsn what's about -renamesourcefileattribute SourceFile? it's been removed but it was in the official docs.

Sorry I've been meaning to leave some comments here but got lost in testing the changes 😄

So I thought that we probably don't need -renamesourcefileattribute because our sdks are open-source, and there's no reason to hide the names of the files - I think this is something the consumers can decide whether they want to hide filenames from stacktraces or not and add this on themselves. This doesn't break de-obfuscation or anything, just there for security reasons (to make it harder to find out what files are causing problems)

@romtsn
Copy link
Member

romtsn commented May 24, 2022

@marandaneto should I approve it since I can't add you as a reviewer? 🤣

Also, this is what's kept at the moment (I think we did not miss anything and I gave it thorough testing, including bytecode manipulation stuff - still works, because it happens pre-R8 phase)
image

@romtsn romtsn merged commit 1bd5185 into 6.x.x May 25, 2022
@romtsn romtsn deleted the chore/improve-proguard-rules branch May 25, 2022 13:40
@PaulWoitaschek
Copy link
Contributor

This crashes on release builds on AGP 7.2.1 due to the NdkIntegration using reflection.

@marandaneto
Copy link
Contributor Author

This crashes on release builds on AGP 7.2.1 due to the NdkIntegration using reflection.

Thanks for the report, would you mind raising a new issue with more context and stack trace? Thanks.

@romtsn
Copy link
Member

romtsn commented Jun 11, 2022

Yeah we'd need more details - we explicitly keep SentryNdk class, so unless there's a rule which removes it, it should work fine (also, tried to reproduce - no luck)

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.

4 participants