Skip to content

Conversation

@Syhids
Copy link

@Syhids Syhids commented Oct 4, 2017

Fixes GH-510

@bretthoerner
Copy link

bretthoerner commented Oct 4, 2017

Thanks for doing this!

Were you able to build and test this? If you app is compiled with an SDK below 16 wouldn't compilation fail because the field doesn't even exist?

@Syhids
Copy link
Author

Syhids commented Oct 4, 2017

@bretthoerner thanks for your time :)

Yes, I think compilation will fail if you set compileSdkVersion to 15, but you normally want to compile with the latest SDK to have the posibility of calling new SDK methods, but guarded in version checks, like this PR does.

Honestly, I didn't built the module. I tried to generate a snapshot using jitpack, but I think it doesn't recognize the modules. When I have more time, I'll try to test it properly :)

@bretthoerner
Copy link

Cool, if I get time I'll try it in an emulator with an old API.

@friederbluemle
Copy link

Thanks @Syhids.

After checking the official documentation, I can confirm that totalMem was only added in SDK level 16, which is why it has to be guarded from being accessed on devices with SDK < 16.
@bretthoerner What is the minimum supported SDK level of sentry-android? I checked the pom file, and I saw a property <android.version>4.1.1.4</android.version> - I guess that means that the code is compiled against this version (16) of Android.

Usually, libraries also define a minimum SDK - If an app tries to include a library with a higher minimum version than the app itself, compilation will fail.

@bretthoerner
Copy link

Yeah, we compile/test against 16. But if we can lower the requirements this easily then I don't see a reason not to.

How would we declare the minimum in the POM?

if (memInfo != null) {
deviceMap.put("free_memory", memInfo.availMem);
deviceMap.put("memory_size", memInfo.totalMem);
if (Build.VERSION.SDK_INT >= 16) {

Choose a reason for hiding this comment

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

Use Build.VERSION_CODES.JELLY_BEAN instead of the plain (magic) number 16 here. Other than that, it looks good!

Copy link
Author

Choose a reason for hiding this comment

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

Sure! Thanks :)

@friederbluemle
Copy link

friederbluemle commented Oct 9, 2017

@bretthoerner Good question, I briefly checked, and could not find a straightforward answer how to set the minimum SDK when using Maven. With Gradle, it would be minSdkVersion in build.gradle.

I can check again in the coming days. In the meantime, once my comment is addressed, this PR should be merged as it definitely avoids a crash on SDK 15 and below.

@bretthoerner
Copy link

Great, thanks to both of you.

@bretthoerner
Copy link

@Syhids if you can make that change I'll merge, thanks!

@bretthoerner bretthoerner merged commit dfc86e0 into getsentry:master Oct 9, 2017
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.

3 participants