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

Fixed the run-android cli script to start the correct activity when using different applicationId for different variants #8950

Closed
wants to merge 1 commit into from

Conversation

kgritesh
Copy link

This is a small fix in the runAndroid that instead of using the AndroidManifest file from the src at app/src/main/AndroidManifest.xml uses the manifest file that is generated during the build process to find out the correct package name and activity name. As a result, it solves the issue where runAndroid command fails to start the app after installing it.
Relevant Issue: #5546

@ghost
Copy link

ghost commented Jul 21, 2016

By analyzing the blame information on this pull request, we identified @burgalon and @jimthedev to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 21, 2016
@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 21, 2016
@mkonicek
Copy link
Contributor

mkonicek commented Jul 31, 2016

From the issue:

I saw that for the command react-native run-android you guys are getting the package name of the application out of app/src/main/AndroidManifest.xml. This is ok if you change the application package in the gradle file AND in the AndroidManifest.xml.

What's the reason to change only one and not the other? Do you have an example when you'd do that?

This PR contains a lot of code, is there a way to solve this say with a few lines of code?

One thing not clear from the description of this PR: what's the new --activity argument for? When would someone use it?

@tume
Copy link

tume commented Aug 2, 2016

Sorry if i'm discussing in wrong place but I also had problems with applicationId as described and this pull request came closest when searching.

Our case is that we have same application built for different environments (dev/qa/prod) and we need to install those side-by-side so that is the reason for different applicationIds. If I have understood correctly the applicationId should be decoupled from the Androidmanifest.xml package_name exactly for this kind of purpose.

See more: http://tools.android.com/tech-docs/new-build-system/applicationid-vs-packagename

Not experienced enough on Android build systems to comment on actual implementation but would think that simpler option would be just to read straight the applicationId from build.gradle if there is no other problems with it.

@ghost
Copy link

ghost commented Aug 20, 2016

@kgritesh updated the pull request - view changes

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 20, 2016
const ret = splitVariant(gradleFilePath, variant);
const buildType = ret[0];
const flavor = ret[1];
const paths = ['android/app/build/intermediates/manifests/full'];

Choose a reason for hiding this comment

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

Since we are already in the android directory: https://github.com/facebook/react-native/pull/8950/files#diff-a003f89d34db18b4567fdb55080a003eR81

I believe these two file paths should be app/build.gradle and app/build/intermediates/manifests/full

Copy link
Author

Choose a reason for hiding this comment

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

@enterline yes you are right these two paths should be app/build.gradle and app/build/intermediates/manifests/full . I have updated my PR accordingly.

@facebook-github-bot
Copy link
Contributor

@kgritesh updated the pull request - view changes

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 23, 2016
@ghost
Copy link

ghost commented Aug 23, 2016

@kgritesh updated the pull request - view changes

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 23, 2016
@ghost ghost added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 30, 2016
@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 7, 2016
@ghost
Copy link

ghost commented Sep 20, 2016

@kgritesh updated the pull request - view changes

@kgritesh
Copy link
Author

@mkonicek sorry for missing your comments earlier.
Android has concept of product flavors where for each flavour you can define a different package name inside build.gradle but the AndroidManifest.xml will still only have package name for your default flavor. For eg for our app, we have a product flavor called staging and we add a .staging to the package name , however android manifest file has the base package name, hence as the issue suggests its better to get the package name from the generated AndroidManifest file and not the default one

As per your other comment about that this pr contains lot of code, i don't think there is a simpler and cleaner way to solve this for all kind of corner cases

Finally the --activity options allows you to pass name of your launch activity incase its not called MainActivity. Most common use case is when you are adding react native to an existing app and the default launch activity is named something else

@ghost
Copy link

ghost commented Sep 20, 2016

@kgritesh updated the pull request - view changes

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 20, 2016
@StevePotter
Copy link
Contributor

💯 . This fixed my problem. Our build.gradle looks like:

    buildTypes {
        debug {
            applicationIdSuffix '.debug'
            versionNameSuffix '-DEBUG'
        }
        staging {
            applicationIdSuffix '.staging'
            versionNameSuffix '-STAGING'
        }
}

And so on. This PR fixes it perfectly. I worked like mad on #10178 and it just got merged. So should this one. The build scripts in react-native are weak and people like @kgritesh are improving it. Let's get this merged because right now I have to patch it. Thanks @kgritesh for your hard work!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 17, 2016
@kgritesh
Copy link
Author

thanks @StevePotter for your kind words. I am also waiting for this to get merged.

@facebook-github-bot
Copy link
Contributor

@kgritesh updated the pull request - view changes

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 18, 2016
@VictoriaQ
Copy link

This fixed our issues, thanks!

Note that (at least in Linux), if you run it with react-native run-android --variant=myflavorDebug it will complain because it cannot find 'app/build/intermediates/manifests/full/myflavor/Debug/AndroidManifest.xml.

Running the task with the variant name in lowercase react-native run-android --variant=myflavordebug, works, however.

I think that buildType should be converted to lowercase in getManifestFile.

@hramos
Copy link
Contributor

hramos commented Nov 11, 2016

@mkonicek thoughts on this PR?

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
using different applicationId for different variants
@chetstone
Copy link
Contributor

@mkonicek Any progress on merging this PR?

@fkrauthan
Copy link

Yeah what is going on? This merge request is super old. I already opened a bug about exactly this but it seems like no love for Android in react native :(

@hramos
Copy link
Contributor

hramos commented Jan 19, 2017

Thanks for the PR. I'd like to push for solving this issue with fewer lines of code as well.

@kgritesh
Copy link
Author

kgritesh commented Jan 20, 2017 via email

@hramos
Copy link
Contributor

hramos commented Jan 20, 2017

Does it need to be a new PR or can you land the commits on this one?

@vongohren
Copy link

Hows the work with this going?

@vongohren
Copy link

Is this getting anywhere? @kgritesh?
Or is there any other way of getting different environmentapps on the phone?
I would like to get 3 apps on my phone, dev, staging and prod. But currently this is not working with react beacuse it does not support applicationIdSuffix.

Can this be solved with any other solution? @hramos @mkonicek
I guess you guys have environmentapps on your projects?

@hramos
Copy link
Contributor

hramos commented Feb 22, 2017

@Snorlock @chuyik the original author has not responded in a while. If you're interested in fixing this, feel free to send a PR.

@kgritesh
Copy link
Author

kgritesh commented Feb 22, 2017 via email

@vongohren
Copy link

It was closed because?

@hramos
Copy link
Contributor

hramos commented Mar 8, 2017

@Snorlock the author stated multiple times they will open a new PR that addresses the feedback provided above.

@kgritesh
Copy link
Author

kgritesh commented Mar 9, 2017

@Snorlock new PR submitted at #12820

Bronsequences added a commit to Bronsequences/react-native that referenced this pull request Aug 28, 2017

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet