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: #137 #144

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix: #137 #144

wants to merge 3 commits into from

Conversation

SilverGhostBS
Copy link

@SilverGhostBS SilverGhostBS commented Mar 25, 2020

Hello, just a naive attempt at trying to refactor this for clarity as per issue #137

Feel free to use or not, hope I didn't forget anything.


This change is Reviewable

@msftclas
Copy link

msftclas commented Mar 25, 2020

CLA assistant check
All CLA requirements met.

Updated dependencies
Updated to Gradle 5.6.4
Updated to Gradle Android Plugin 3.6.0
Added publishing for android_tor_installer
Copy link
Member

@yaronyg yaronyg left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, 4 of 6 files at r2.
Reviewable status: 7 of 9 files reviewed, 2 unresolved discussions (waiting on @SilverGhostBS)


android/build.gradle, line 13 at r2 (raw file):

    }
    dependencies {
        classpath "com.android.tools.build:gradle:3.6.0"

Shouldn't this keep using the variable rather than hard coding?


android_tor_installer/build.gradle, line 13 at r2 (raw file):

    }
    dependencies {
        classpath "com.android.tools.build:gradle:3.6.0"

Same comment from other gradle file

@sisbell
Copy link
Contributor

sisbell commented Mar 26, 2020


android/build.gradle, line 15 at r2 (raw file):

        classpath "com.android.tools.build:gradle:3.6.0"
    }
}

Do we need jcenter as a repo? jcenter seems to function as a proxy for multiple repos so I've seen the same artifiact (based on maven coordinates) have different sha values depending on whichever repo is hit.

@SilverGhostBS
Copy link
Author

SilverGhostBS commented Mar 26, 2020 via email

@SilverGhostBS
Copy link
Author

Reviewed 3 of 3 files at r1, 4 of 6 files at r2.
Reviewable status: 7 of 9 files reviewed, 2 unresolved discussions (waiting on @SilverGhostBS)

android/build.gradle, line 13 at r2 (raw file):

    }
    dependencies {
        classpath "com.android.tools.build:gradle:3.6.0"

Shouldn't this keep using the variable rather than hard coding?

android_tor_installer/build.gradle, line 13 at r2 (raw file):

    }
    dependencies {
        classpath "com.android.tools.build:gradle:3.6.0"

Same comment from other gradle file

Yes, I agree having it as a variable makes sense but it prevents Android Studio from suggesting you to update when a new version comes out.

@SilverGhostBS
Copy link
Author

Plus it adds an exta "obstacle" when you want to build the android module separately from the rest

Copy link
Member

@yaronyg yaronyg left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 6 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SilverGhostBS)

@sisbell
Copy link
Contributor

sisbell commented Apr 2, 2020


gradle.properties, line 2 at r2 (raw file):

# Configure Android Gradle Plugin Version
androidplugin=3.6.0

We would need at least gradle 5.6.4 for this version of the android plugin. The current configuration in the gradle wrapper is for 4.6. This would break the command line build.

@jollyjoker992
Copy link

@SilverGhostBS Can you fix the conflict. This PR will help us a lot for the android lib. Thanks.

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.

5 participants