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

Fix Android dependencies and build errors #2720

Merged

Conversation

Projects
None yet
7 participants
@friederbluemle
Copy link
Contributor

commented Mar 3, 2019

Does any other open PR do the same thing?

No

What issue is this PR fixing?

Currently, the yarn tasks start:android and run:android to build and run the Android version are failing on the master branch with the following error:

* What went wrong:
A problem occurred evaluating project ':react-native-maps-lib'.
> Could not find method compileOnly() for arguments [com.facebook.react:react-native:+] on object of type org.gradle.api.internal.artifacts.dsl.dependencies.DefaultDependencyHandler.

This is caused by partially and incorrectly updated dependencies of the Android build system, which was addressed by updating the dependencies as described below. Many obsolete and unused leftover files of old Gradle wrapper installations and a couple of files from the Eclipse days were removed as well.

The three main things this PR addresses is:

  • Update Gradle wrapper to 5.2.1 (latest stable version)
  • Update Android Gradle plugin to 3.2.1 (version that ships with latest version of React Native)
  • Remove unused appcompat-v7 dependency from library project

See individual commits for more details.

How did you test this PR?

react-native start
yarn run-android
yarn run-ios

Correct behavior (build and run) was verified using an Android device, an Android emulator, and an iOS simulator.

Includes and closes #2712
Also fixes #2663

Once approved, please use a normal GitHub merge (i.e. NO rebase/squash merge) to integrate the commit(s) from the branch of this pull request. The changes are broken up into meaningful, atomic commits, and my branch should already be up-to-date with the latest target branch. If it isn't, or if you'd like me to combine something, please let me know, and I will make the necessary updates as soon as possible. Thank you!

@alvelig

This comment has been minimized.

Copy link
Collaborator

commented Mar 3, 2019

LGTM @rborn 🐽

@friederbluemle friederbluemle force-pushed the friederbluemle:fix-android-build branch from 5921602 to 4f6c684 Mar 4, 2019

@radeno

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@friederbluemle
I like this PR, when i created first PR for react-native-maps it was pain for me to run test environment. Almost everything was outdated and not working. So i am welcoming it.

What i recommend by mine good practice is to separate iOS and Android specific changes as two PRs. It could be checked and tested separately. This PR is not change in couple files, but 26. It could helps to merge it faster when it will be as two independent pull requests.

Do you think use Gradle 5.2.x is good idea? RN 0.58 uses 4.10.2, even RN0.59 will use 4.10.2. Gradle 5.0 lands in RN master branch just 12 days ago. I think 4.10 is safe now.

@radeno

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Anyway iOS changes looks good to me.
All chagnes are created by Cocoapods automatic scripts. So its safe.

@friederbluemle friederbluemle force-pushed the friederbluemle:fix-android-build branch from 4f6c684 to 39590ec Mar 5, 2019

@friederbluemle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@radeno - Thanks a lot for the review and the comment! 👍

As you suggested, I downgraded the Gradle wrapper to 4.10.3 (like you said, RN currently ships with the 4.10.x version)

I also excluded the ios changes (Podfile.lock) - I'll open a separate PR once this one is in. It should make the review a bit easier.

@friederbluemle friederbluemle force-pushed the friederbluemle:fix-android-build branch from 39590ec to 58f8bcc Mar 5, 2019

@radeno

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@friederbluemle i'm not an Android expert so lets get inspiration from https://github.com/react-native-community/react-native-netinfo which is community repo from other community repos taking inspiration in Lean Core initiative.

All gradle files from root folder should be deleted or moved to right place. We need only related for build in lib/android and example directory example/android, It is little bit mess, because gradle config files are not separated by scope "build" or "example". But there is third scope, global. Which is what you don't bring with this PR, it is existed legacy. So lets clean it up.

Can you check it please?

@friederbluemle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@radeno - Yes good point. Do you suggest we should also do it in this PR as well or do keep it separate? As you mentioned I am not introducing any new legacy tech debt with this PR (only clean up, removing files, and updating/moving existing files).

I agree there is more follow up work to be done. I have some pending wip on a separate branch locally that moves the wrapper from the root directory (I think that should be taken care of separately).

@radeno

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@friederbluemle in my opinion it should be good move to transfer them into correct directories by this PR.
Running examples on actual Android studio is broken anyway without your PR. And removing global Gradle scope is nice to have.

Most gradle files from root directory belongs to example directory. So i think, yes lets move it.

friederbluemle added some commits Feb 28, 2019

Remove obsolete files and folders
- Delete compiled files
- Remove obsolete Gradle wrapper leftover
- Remove Eclipse project leftover
Fix Android dependencies and build errors
- Update Gradle wrapper to 4.10.3
- Update Android Gradle plugin to 3.2.1
- Remove unused appcompat-v7 dependency from lib project

@friederbluemle friederbluemle force-pushed the friederbluemle:fix-android-build branch from 58f8bcc to 8811ba3 Mar 8, 2019

@friederbluemle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

@radeno - To make the PR review easier and lower the impact/risk, would it be possible to use two PRs? This PR here does not create any new files, just updates existing ones. I will open a follow up PR right after this on is in. We are already successfully using an internal fork with the changes on this PR.

@radeno

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@friederbluemle for me it doesnt matter because test case is easy. Will it work or will not :) but i am not maintaner.
Cc @alvelig @rborn

@alvelig

This comment has been minimized.

Copy link
Collaborator

commented Mar 8, 2019

@rborn

This comment has been minimized.

Copy link
Collaborator

commented Mar 8, 2019

@christopherdro christopherdro merged commit d835964 into react-native-community:master Mar 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grifotv

This comment has been minimized.

Copy link

commented Mar 9, 2019

@friederbluemle unfortunately I'm unable to run react-native run-android since this has been merged.

info Building and installing the app on the device (cd android && ./gradlew app:installDebug)...

> Configure project :app
Reading env from: .env.development

FAILURE: Build failed with an exception.

* Where:
Script '/my_project_path/node_modules/react-native-maps/lib/android/gradle-maven-push.gradle' line: 48

* What went wrong:
A problem occurred configuring project ':react-native-maps'.
> Could not get unknown property 'GROUP' for object of type org.gradle.api.publication.maven.internal.deployer.DefaultGroovyMavenDeployer.

  • react-native: next
  • react-native-maps: https://github.com/react-community/react-native-maps.git

Should I create an issue?

Thanks

@friederbluemle friederbluemle deleted the friederbluemle:fix-android-build branch Mar 9, 2019

@friederbluemle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

@grifotv Thanks for the report. I'm checking now!

@friederbluemle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

Confirming the build error that @grifotv reported.

I missed including a small change in this PR that I had locally. Sorry about that.

I opened a PR with the fix here: #2727

@christopherdro @alvelig @rborn - Please check, hopefully we can get this merged quickly. Thanks and sorry again for the oversight!

@alvelig

This comment has been minimized.

Copy link
Collaborator

commented Mar 9, 2019

Merged

@rborn

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2019

@friederbluemle it looks like some people still have issues with a missing line in build.gradle
implementation "com.android.support:appcompat-v7:${rootProject.ext.supportLibVersion}"

#2695 (comment)

This PR only includes it in example, but not in the lib/android/build.gradle

Do we need to add it there or they are using an older version of the module? ( didn't have time to test :( )
Thanks

@friederbluemle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

@rborn Thanks for the heads up! Looking into it!

@friederbluemle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

@rborn - Looking at the comments in the other thread, these people were not using the master branch of react-native-maps (i.e. the build.gradle file posted looks different than the one currently on master).

I checked the code again, and there are definitely no dependencies on appcompat-v7 in the library project.

However, there are a couple of dependencies on android.support.v4 in AirMapView. So, technically support-v4 should explicitly be declared as a dependency in build.gradle. For now, it's probably best to re-add appcompat-v7. See #2743.
Note that support-v4 is also pulled in transitively by many other libraries, for example play-services-base:

$ ./gradlew :react-native-maps-lib:dependencies

[...]
+--- com.google.android.gms:play-services-base:16.1.0
|    +--- com.google.android.gms:play-services-basement:16.2.0
|    |    \--- com.android.support:support-v4:26.1.0
[...]

The issues people are having are most likely related to the way dependencies are declared in the app project, and the resulting conflicts.

https://docs.gradle.org/current/userguide/customizing_dependency_resolution_behavior.html

Other possible solutions/workarounds include:

    implementation ('com.facebook.react:react-native:+') {
        exclude group: 'com.android.support', module: 'support-v4'
    }

or

subprojects {
  project.configurations.all {
    resolutionStrategy.eachDependency { details ->
      if (details.requested.group == 'com.android.support'
          && details.requested.name == 'support-v4') {
        details.useVersion '28.0.0'
      }
    }
  }
}

Ideally there should be no need to modify files inside of the node_modules folder!

@rborn

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2019

@friederbluemle thank you very much for the investigation. Let's keep it like his for now and keep an eye on it.
I expect to upgrade a project to last RN soon so I'll also test first hand.

Thanks again ❤️

@arosendorff

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Sorry if I missed it, but is there a reason why this PR takes out the ability of react-native-maps to use the googlePlayServicesVersion in your top level gradle file? That ability was added in PR #2047

@friederbluemle

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@arosendorff - Good question!

The library (and its source code) is built against and guaranteed to work with one specific version of the play-services-* artifacts. The same is true for all other dependencies. Therefore, making it possible to externally modify the dependency versions used (without looking and checking the source code) leads to brittle and unpredictable builds.

However: It does seem like this unfortunately is the prevalent, "recommended", and only (?) way dependencies of React Native apps are handled, at least at the moment.
In many cases, "small" updates (i.e. the patch component) will work without issues and don't require code updates. I assume the main reason for the desire to set this externally is to avoid certain types of dependency mismatch errors during the build. Being able to adjust the versions with the ext variables offers a "simple" solution to align transitive dependencies used by several referenced packages, though there is no guarantee it will work, and there are other ways to prevent mismatch errors as well.

In any case, I do admit this was a mistake on my end, and it's not the job of this repository/library to change the way React Native dependencies are consumed, so I've added this functionality back in this PR right here: #2765

@rborn

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2019

@arosendorff #2765 was merged, please test with the last master. Thanks for your observation 🤗

@arosendorff

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@friederbluemle I get what you're saying about the required play-services-* artifacts. Perhaps it's worth mentioning in the readme that while the module will honour your global configuration, there is a recommended version you should use to ensure everything runs smoothly?

@rborn Perfect, thanks so much! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.