Skip to content
This repository was archived by the owner on Mar 3, 2020. It is now read-only.

Add minSdkVersion to AndroidManifest.xml #12

Merged
merged 1 commit into from
Jan 12, 2016

Conversation

astuetz
Copy link

@astuetz astuetz commented Jan 8, 2016

Not setting a minSdkVersion causes the Android build system to automatically add the READ_PHONE_STATE permission. (Default value of minSdkVersion and targetSdkVersion is 3)

From and Mainfest.permission docs:

Note: If both your minSdkVersion and targetSdkVersion values are set to 3 or lower, the system implicitly grants your app this permission. If you don't need this permission, be sure your targetSdkVersion is 4 or higher.

http://developer.android.com/reference/android/Manifest.permission.html#READ_PHONE_STATE

Unfortunately I haven't been able to test this, as building android-jsc gives me a ton of errors, I guess the buildscript has to be adjusted a bit.

We initially saw this permission popping up on our app after adding the react-native dependency.

@ghost
Copy link
Author

ghost commented Jan 12, 2016

@kmagiera any chance this can get merged soon?

astreet added a commit that referenced this pull request Jan 12, 2016
Add minSdkVersion to AndroidManifest.xml
@astreet astreet merged commit 488e0b9 into facebookarchive:master Jan 12, 2016
@astreet
Copy link
Contributor

astreet commented Jan 12, 2016

Thanks!

@ghost
Copy link
Author

ghost commented Jan 12, 2016

@astreet thanks for merging it in, any idea on when you're going to push a new version to maven central?

@astreet
Copy link
Contributor

astreet commented Jan 12, 2016

Ah, I'm not sure what the process is for that or who's maintaining it here, @kmagiera do you know?

@astuetz astuetz deleted the min-sdk-version branch January 12, 2016 17:58
@kmagiera
Copy link
Contributor

It was actually @brentvatne who pushed it to maven last time. As you can see there is not much stuff happening in this repo so we haven't been updating it since the initial release and don't have any process around releasing it.

@brentvatne
Copy link
Contributor

@astreet - I can push this next week if you like

@kmagiera
Copy link
Contributor

The question is what will we put as version number as atm it refers to JSC version number (from SVN) and JSC is not going to be updated this time

@astreet
Copy link
Contributor

astreet commented Jan 19, 2016

Oh, yeah we should be using a version number specific to this repo and not exactly the JSC version number (which should be able to be derived from our own version number).

@ghost
Copy link
Author

ghost commented Jan 27, 2016

@brentvatne do you have any updates about when you can publish a new version?

@bestander
Copy link

ping, is it published, @brentvatne?

@mikelambert
Copy link

So as I understand it, this fix has been withheld for ten months, because of a concern about the version number needing to match the original JSC version?

Looks like the current version from Sept 2015 is r174650:
https://mvnrepository.com/artifact/org.webkit/android-jsc

Can't you just publish this as r174650.1 and be done with it?

@hey99xx
Copy link

hey99xx commented Mar 9, 2017

Ping, is there anything blocking this? Did Facebook forget that this package exists? I understand updating JavaScriptCore version is a hard task; but all this needs is one publish to Maven

@bestander
Copy link

bestander commented Mar 9, 2017

Try reaching out to maintainers via twitter

@mikelambert
Copy link

@astreet @brentvatne @mkonicek (in case this works any better than twitter)

@hey99xx
Copy link

hey99xx commented Mar 11, 2017

I'm not going to open a Twitter account just for this library, but thanks for the suggestion. Since the repository is under facebook namespace in Github, I assumed this is the correct place to ping it.

@mkonicek
Copy link
Contributor

Wow this permission looks really scary:

Allows read only access to phone state, including the phone number of the device, current cellular network information, the status of any ongoing calls, and a list of any PhoneAccounts registered on the device.

@mkonicek
Copy link
Contributor

I'll ping @brentvatne.

@mkonicek
Copy link
Contributor

Ah sorry, just read facebook/react-native#12965 which works around this - should be good using that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants