Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

Conversation

@marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Oct 23, 2019

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Init. NDK only if sdk opt is enabled.

💡 Motivation and Context

Right now is trying to load every time.

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing

🔮 Next steps

Write tests

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

One comment to confirm.
Also, we need to create dir now on a new PR?
Otherwise lgtm.

@codecov-io
Copy link

codecov-io commented Oct 24, 2019

Codecov Report

Merging #60 into master will decrease coverage by 5.96%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #60      +/-   ##
============================================
- Coverage     44.66%   38.69%   -5.97%     
+ Complexity      243      185      -58     
============================================
  Files            52       51       -1     
  Lines          1442     1367      -75     
  Branches         90       78      -12     
============================================
- Hits            644      529     -115     
- Misses          766      811      +45     
+ Partials         32       27       -5
Impacted Files Coverage Δ Complexity Δ
...re/src/main/java/io/sentry/core/SentryOptions.java 73.91% <ø> (-5.75%) 21 <0> (-8)
...ain/java/io/sentry/core/protocol/SentryThread.java 0% <0%> (-81.49%) 0% <0%> (-14%)
...ore/src/main/java/io/sentry/core/SentryValues.java 0% <0%> (-50%) 0% <0%> (-1%)
...java/io/sentry/core/protocol/SentryStackTrace.java 0% <0%> (-44.45%) 0% <0%> (-2%)
...java/io/sentry/core/protocol/SentryStackFrame.java 0% <0%> (-19.3%) 0% <0%> (-6%)
sentry-core/src/main/java/io/sentry/core/Hub.java 18.18% <0%> (-9.71%) 4% <0%> (-4%)
...core/src/main/java/io/sentry/core/SentryEvent.java 18.39% <0%> (-9.7%) 6% <0%> (-6%)
...entry-core/src/main/java/io/sentry/core/Scope.java 62.06% <0%> (-6.9%) 11% <0%> (-1%)
...ore/src/main/java/io/sentry/core/SentryClient.java 70.27% <0%> (-4.73%) 9% <0%> (-3%)
... and 2 more

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 3fe98c7...1ccb641. Read the comment docs.

options.setCacheDirPath(envelopesDir.getAbsolutePath());
}

private static boolean isNdkAvailable() {
Copy link

Choose a reason for hiding this comment

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

Are we settling on the fact that NDK support will not be available on API < 21?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ninniuz for the first release, probably yes, we have a system blocker and trying to find a workaround.
JFYI dl_iterate_phdr method only available API >= 21
https://android.googlesource.com/platform/bionic/+/master/docs/status.md

Copy link

Choose a reason for hiding this comment

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

Yeah, tried to build the NDK feature branch on API 14 and got an error on that method :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we might have a workaround, parsing /proc/PID/maps by hand and getting what we need.
but still thinking if it's a good idea.

@marandaneto
Copy link
Contributor Author

@bruno-garcia the configuration will be to avoid a reflection call if we already know NDK is not there or disabled.
Reflection on Android is expensive.

@bruno-garcia bruno-garcia merged commit 82517a3 into master Oct 25, 2019
@bruno-garcia bruno-garcia deleted the enha/init_ndk_by_manifest branch October 25, 2019 14:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants