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

Add minSdkVersion to AndroidManifest.xml #12

Merged
merged 1 commit into from Jan 12, 2016

Conversation

Projects
None yet
9 participants
@astuetz
Contributor

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.

@astuetz

This comment has been minimized.

Show comment
Hide comment
@astuetz

astuetz Jan 12, 2016

Contributor

@kmagiera any chance this can get merged soon?

Contributor

astuetz commented Jan 12, 2016

@kmagiera any chance this can get merged soon?

astreet added a commit that referenced this pull request Jan 12, 2016

Merge pull request #12 from astuetz/min-sdk-version
Add minSdkVersion to AndroidManifest.xml

@astreet astreet merged commit 488e0b9 into facebook:master Jan 12, 2016

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Jan 12, 2016

Contributor

Thanks!

Contributor

astreet commented Jan 12, 2016

Thanks!

@astuetz

This comment has been minimized.

Show comment
Hide comment
@astuetz

astuetz Jan 12, 2016

Contributor

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

Contributor

astuetz 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

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Jan 12, 2016

Contributor

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

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 astuetz:min-sdk-version branch Jan 12, 2016

@kmagiera

This comment has been minimized.

Show comment
Hide comment
@kmagiera

kmagiera Jan 13, 2016

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.

Contributor

kmagiera commented Jan 13, 2016

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

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jan 13, 2016

Contributor

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

Contributor

brentvatne commented Jan 13, 2016

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

@kmagiera

This comment has been minimized.

Show comment
Hide comment
@kmagiera

kmagiera Jan 14, 2016

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

Contributor

kmagiera commented Jan 14, 2016

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

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Jan 19, 2016

Contributor

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).

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).

@astuetz

This comment has been minimized.

Show comment
Hide comment
@astuetz

astuetz Jan 27, 2016

Contributor

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

Contributor

astuetz commented Jan 27, 2016

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

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander May 12, 2016

ping, is it published, @brentvatne?

bestander commented May 12, 2016

ping, is it published, @brentvatne?

@mikelambert

This comment has been minimized.

Show comment
Hide comment
@mikelambert

mikelambert Oct 6, 2016

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?

mikelambert commented Oct 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@hey99xx

hey99xx 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

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

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Mar 9, 2017

Try reaching out to maintainers via twitter

bestander commented Mar 9, 2017

Try reaching out to maintainers via twitter

@mikelambert

This comment has been minimized.

Show comment
Hide comment
@mikelambert

mikelambert Mar 9, 2017

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

mikelambert commented Mar 9, 2017

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

@hey99xx

This comment has been minimized.

Show comment
Hide comment
@hey99xx

hey99xx 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.

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

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 16, 2017

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.

Contributor

mkonicek commented Mar 16, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 16, 2017

Contributor

I'll ping @brentvatne.

Contributor

mkonicek commented Mar 16, 2017

I'll ping @brentvatne.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 16, 2017

Contributor

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

Contributor

mkonicek commented Mar 16, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment