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

Upgrade Gradle to 7.4 & AGP to 7.1.1 (& Disable Jetifier & Dependency Removal) #18

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion android-exoplayer/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ dependencies {
implementation('com.google.android.exoplayer:extension-okhttp:2.13.3') {
exclude group: 'com.squareup.okhttp3', module: 'okhttp'
}
implementation 'com.squareup.okhttp3:okhttp:${OKHTTP_VERSION}'

Choose a reason for hiding this comment

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

Would the better fix here be to just change this to double-quotes? We could also do a PR to push this fix upstream, we're not the first to notice it: TheWidlarzGroup@3dc607c.

Just for reference: here is the PR adding this line to react-native-video: TheWidlarzGroup#2340

Copy link
Author

Choose a reason for hiding this comment

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

👋 @mchowning !

Thanks for the response! 🥇

Would the better fix here be to just change this to double-quotes?

I actually tried that but it doesn't work as the OKHTTP_VERSION is not available, it only resides within the RN note module. Thus, the build failed when I tried.

My question is if we actually need this dependency, since we are able to build even without it. 🤔

We could also do a PR to push this fix upstream, we're not the first to notice it: TheWidlarzGroup@3dc607c.

We can try doing that, yes. 👍

Just for reference: here is the PR adding this line to react-native-video: TheWidlarzGroup#2340

👍

Choose a reason for hiding this comment

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

I actually tried that but it doesn't work as the OKHTTP_VERSION is not available, it only resides within the RN note module. Thus, the build failed when I tried.

Ah good point. We could always wrap this in a project.hasProperty(...), but keeping it simple here by just removing it also seems fine since we obviously haven't needed it.

If you want to go with the "removal" approach you have here, I'm curious if you have any thoughts about possibly commenting out this line and adding a comment describing why it is being commented out. We generally try to keep these forks as close to the source as possible, and I'm thinking it might be hard for someone to backtrack and figure out why we removed this line when the inevitable conflict on this line arises in the future. I don't feel strongly, I'm more just thinking out loud, so let me know what you think.

Choose a reason for hiding this comment

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

It builds without this dependency because okhttp is a transitive dependency of com.facebook.fresco:imagepipeline. If this project is using okhttp directly, we should keep it and use the version we want. If the project is not using it directly, it should be fine to remove it as whichever dependency needs it should also have it in its pom file.

Copy link
Author

@ParaskP7 ParaskP7 Feb 28, 2022

Choose a reason for hiding this comment

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

👋 @mchowning @oguzkocer !

Thank you so much for your input! 🙏

Ah good point. We could always wrap this in a project.hasProperty(...), but keeping it simple here by just removing it also seems fine since we obviously haven't needed it.

This is a good idea. 👍

If you want to go with the "removal" approach you have here, I'm curious if you have any thoughts about possibly commenting out this line and adding a comment describing why it is being commented out. We generally try to keep these forks as close to the source as possible, and I'm thinking it might be hard for someone to backtrack and figure out why we removed this line when the inevitable conflict on this line arises in the future. I don't feel strongly, I'm more just thinking out loud, so let me know what you think.

Yes, this is a good argument, on keeping the forks as close to the source as possible, I agree. 👍

It builds without this dependency because okhttp is a transitive dependency of com.facebook.fresco:imagepipeline.

Yes, actually after running ./gradlew dependencies and checking the before and after result, com.squareup.okhttp3:okhttp-urlconnection is also using okhttp as a transitive dependency, as the whole androidx.appcompat:appcompat dependency itself.

I also did a quick analysis, using ./gradlew dependencies, a before and after diff for the removal of the explicit 'com.squareup.okhttp3:okhttp:${OKHTTP_VERSION}' dependency and the only diff I found are the below line, which is found twice on the top level and the com.facebook.react:react-native:0.66.2 tree:

+--- com.squareup.okhttp3:okhttp:${OKHTTP_VERSION} -> 4.9.1 (*)

This makes me think that:

  1. The ${OKHTTP_VERSION} is being hardcoded and doesn't change to the OKHTTP_VERSION=3.12.12 as defined by the gradle.properties of the ReactAndroid project. Thus, it seems to me that Gradle just ignores while building and then Lint produces the Incorrect Interpolation error.
  2. With or without this incorrectly defined dependency, due to the Incorrect Interpolation error, the end result of building the project is the same. However, this is solely based on building and linting this project individually. If, for some reason, unknown to me due to my lack of React Native experience, when this project is build and used in combination with the ReactAndroid project, it might actually do some code-generation magic to substitute this OKHTTP_VERSION version with the actual version to produce the final result. Thus, then I would no longer feel comfortable removing this dependency.

If this project is using okhttp directly, we should keep it and use the version we want. If the project is not using it directly, it should be fine to remove it as whichever dependency needs it should also have it in its pom file.

Good thinking. 🥇

From what I am seeing this project is using a couple of okhttp3 imports within the DataSourceUtil, which makes me think, just to be on the safe side, since I am not having much React Native experience, to keep this dependency and suppress this Incorrect Interpolation Lint error to avoid future such discussions and have Lint success no matter. Wdyt? 🤔

Commit here: 4fa6833

Choose a reason for hiding this comment

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

From what I am seeing this project is using a couple of okhttp3 imports within the DataSourceUtil, which makes me think, just to be on the safe side, since I am not having much React Native experience, to keep this dependency and suppress this Incorrect Interpolation Lint error to avoid future such discussions and have Lint success no matter. Wdyt?

That doesn't quite feel right to me since we're suppressing a lint warning that is highlighting the fact that this line of code doesn't actually do anything. I don't feel super-strongly, but in light of @oguzkocer 's comment noting that the build only succeeds because okhttp is a transitive dependency (thanks for pointing that out btw), I'd lean toward fixing this with the project.hasProperty(...) wrapper and then switching to using double quotes. That way we can set that variable and actually use it if we want to, and it will be more informative to any developer that bumps into this issue in the future.

Copy link
Author

Choose a reason for hiding this comment

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

👋 @mchowning !

...I'd lean toward fixing this with the project.hasProperty(...) wrapper and then switching to using double quotes.

Yes, I think this is better. The only reason I didn't proceed with this change is because you mentioned that we generally try to keep these forks as close to the source as possible and I wasn't sure If I should proceed with this change.

Please expect me applying this change today. 👍

Copy link
Author

Choose a reason for hiding this comment

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

This is now done: b1bff8b

if (project.hasProperty("OKHTTP_VERSION")) {
implementation "com.squareup.okhttp3:okhttp:${project.property("OKHTTP_VERSION")}"
}
}

afterEvaluate {
Expand Down
2 changes: 1 addition & 1 deletion android-exoplayer/gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ org.gradle.configureondemand=true
org.gradle.caching=true

android.useAndroidX=true
android.enableJetifier=true
android.enableJetifier=false
Binary file modified android-exoplayer/gradle/wrapper/gradle-wrapper.jar
Binary file not shown.
2 changes: 1 addition & 1 deletion android-exoplayer/gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-7.1.1-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-7.4-all.zip
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
282 changes: 172 additions & 110 deletions android-exoplayer/gradlew
Original file line number Diff line number Diff line change
@@ -1,78 +1,129 @@
#!/usr/bin/env sh
#!/bin/sh

#
# Copyright © 2015-2021 the original authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

##############################################################################
##
## Gradle start up script for UN*X
##
#
# Gradle start up script for POSIX generated by Gradle.
#
# Important for running:
#
# (1) You need a POSIX-compliant shell to run this script. If your /bin/sh is
# noncompliant, but you have some other compliant shell such as ksh or
# bash, then to run this script, type that shell name before the whole
# command line, like:
#
# ksh Gradle
#
# Busybox and similar reduced shells will NOT work, because this script
# requires all of these POSIX shell features:
# * functions;
# * expansions «$var», «${var}», «${var:-default}», «${var+SET}»,
# «${var#prefix}», «${var%suffix}», and «$( cmd )»;
# * compound commands having a testable exit status, especially «case»;
# * various built-in commands including «command», «set», and «ulimit».
#
# Important for patching:
#
# (2) This script targets any POSIX shell, so it avoids extensions provided
# by Bash, Ksh, etc; in particular arrays are avoided.
#
# The "traditional" practice of packing multiple parameters into a
# space-separated string is a well documented source of bugs and security
# problems, so this is (mostly) avoided, by progressively accumulating
# options in "$@", and eventually passing that to Java.
#
# Where the inherited environment variables (DEFAULT_JVM_OPTS, JAVA_OPTS,
# and GRADLE_OPTS) rely on word-splitting, this is performed explicitly;
# see the in-line comments for details.
#
# There are tweaks for specific operating systems such as AIX, CygWin,
# Darwin, MinGW, and NonStop.
#
# (3) This script is generated from the Groovy template
# https://github.com/gradle/gradle/blob/master/subprojects/plugins/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt
# within the Gradle project.
#
# You can find Gradle at https://github.com/gradle/gradle/.
#
##############################################################################

# Attempt to set APP_HOME

# Resolve links: $0 may be a link
PRG="$0"
# Need this for relative symlinks.
while [ -h "$PRG" ] ; do
ls=`ls -ld "$PRG"`
link=`expr "$ls" : '.*-> \(.*\)$'`
if expr "$link" : '/.*' > /dev/null; then
PRG="$link"
else
PRG=`dirname "$PRG"`"/$link"
fi
app_path=$0

# Need this for daisy-chained symlinks.
while
APP_HOME=${app_path%"${app_path##*/}"} # leaves a trailing /; empty if no leading path
[ -h "$app_path" ]
do
ls=$( ls -ld "$app_path" )
link=${ls#*' -> '}
case $link in #(
/*) app_path=$link ;; #(
*) app_path=$APP_HOME$link ;;
esac
done
SAVED="`pwd`"
cd "`dirname \"$PRG\"`/" >/dev/null
APP_HOME="`pwd -P`"
cd "$SAVED" >/dev/null

APP_HOME=$( cd "${APP_HOME:-./}" && pwd -P ) || exit

APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`
APP_BASE_NAME=${0##*/}

# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
DEFAULT_JVM_OPTS=""
DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"'

# Use the maximum available, or set MAX_FD != -1 to use that value.
MAX_FD="maximum"
MAX_FD=maximum

warn () {
echo "$*"
}
} >&2

die () {
echo
echo "$*"
echo
exit 1
}
} >&2

# OS specific support (must be 'true' or 'false').
cygwin=false
msys=false
darwin=false
nonstop=false
case "`uname`" in
CYGWIN* )
cygwin=true
;;
Darwin* )
darwin=true
;;
MINGW* )
msys=true
;;
NONSTOP* )
nonstop=true
;;
case "$( uname )" in #(
CYGWIN* ) cygwin=true ;; #(
Darwin* ) darwin=true ;; #(
MSYS* | MINGW* ) msys=true ;; #(
NONSTOP* ) nonstop=true ;;
esac

CLASSPATH=$APP_HOME/gradle/wrapper/gradle-wrapper.jar


# Determine the Java command to use to start the JVM.
if [ -n "$JAVA_HOME" ] ; then
if [ -x "$JAVA_HOME/jre/sh/java" ] ; then
# IBM's JDK on AIX uses strange locations for the executables
JAVACMD="$JAVA_HOME/jre/sh/java"
JAVACMD=$JAVA_HOME/jre/sh/java
else
JAVACMD="$JAVA_HOME/bin/java"
JAVACMD=$JAVA_HOME/bin/java
fi
if [ ! -x "$JAVACMD" ] ; then
die "ERROR: JAVA_HOME is set to an invalid directory: $JAVA_HOME
Expand All @@ -81,92 +132,103 @@ Please set the JAVA_HOME variable in your environment to match the
location of your Java installation."
fi
else
JAVACMD="java"
JAVACMD=java
which java >/dev/null 2>&1 || die "ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH.

Please set the JAVA_HOME variable in your environment to match the
location of your Java installation."
fi

# Increase the maximum file descriptors if we can.
if [ "$cygwin" = "false" -a "$darwin" = "false" -a "$nonstop" = "false" ] ; then
MAX_FD_LIMIT=`ulimit -H -n`
if [ $? -eq 0 ] ; then
if [ "$MAX_FD" = "maximum" -o "$MAX_FD" = "max" ] ; then
MAX_FD="$MAX_FD_LIMIT"
fi
ulimit -n $MAX_FD
if [ $? -ne 0 ] ; then
warn "Could not set maximum file descriptor limit: $MAX_FD"
fi
else
warn "Could not query maximum file descriptor limit: $MAX_FD_LIMIT"
fi
if ! "$cygwin" && ! "$darwin" && ! "$nonstop" ; then
case $MAX_FD in #(
max*)
MAX_FD=$( ulimit -H -n ) ||
warn "Could not query maximum file descriptor limit"
esac
case $MAX_FD in #(
'' | soft) :;; #(
*)
ulimit -n "$MAX_FD" ||
warn "Could not set maximum file descriptor limit to $MAX_FD"
esac
fi

# For Darwin, add options to specify how the application appears in the dock
if $darwin; then
GRADLE_OPTS="$GRADLE_OPTS \"-Xdock:name=$APP_NAME\" \"-Xdock:icon=$APP_HOME/media/gradle.icns\""
fi
# Collect all arguments for the java command, stacking in reverse order:
# * args from the command line
# * the main class name
# * -classpath
# * -D...appname settings
# * --module-path (only if needed)
# * DEFAULT_JVM_OPTS, JAVA_OPTS, and GRADLE_OPTS environment variables.

# For Cygwin or MSYS, switch paths to Windows format before running java
if "$cygwin" || "$msys" ; then
APP_HOME=$( cygpath --path --mixed "$APP_HOME" )
CLASSPATH=$( cygpath --path --mixed "$CLASSPATH" )

JAVACMD=$( cygpath --unix "$JAVACMD" )

# For Cygwin, switch paths to Windows format before running java
if $cygwin ; then
APP_HOME=`cygpath --path --mixed "$APP_HOME"`
CLASSPATH=`cygpath --path --mixed "$CLASSPATH"`
JAVACMD=`cygpath --unix "$JAVACMD"`

# We build the pattern for arguments to be converted via cygpath
ROOTDIRSRAW=`find -L / -maxdepth 1 -mindepth 1 -type d 2>/dev/null`
SEP=""
for dir in $ROOTDIRSRAW ; do
ROOTDIRS="$ROOTDIRS$SEP$dir"
SEP="|"
done
OURCYGPATTERN="(^($ROOTDIRS))"
# Add a user-defined pattern to the cygpath arguments
if [ "$GRADLE_CYGPATTERN" != "" ] ; then
OURCYGPATTERN="$OURCYGPATTERN|($GRADLE_CYGPATTERN)"
fi
# Now convert the arguments - kludge to limit ourselves to /bin/sh
i=0
for arg in "$@" ; do
CHECK=`echo "$arg"|egrep -c "$OURCYGPATTERN" -`
CHECK2=`echo "$arg"|egrep -c "^-"` ### Determine if an option

if [ $CHECK -ne 0 ] && [ $CHECK2 -eq 0 ] ; then ### Added a condition
eval `echo args$i`=`cygpath --path --ignore --mixed "$arg"`
else
eval `echo args$i`="\"$arg\""
for arg do
if
case $arg in #(
-*) false ;; # don't mess with options #(
/?*) t=${arg#/} t=/${t%%/*} # looks like a POSIX filepath
[ -e "$t" ] ;; #(
*) false ;;
esac
then
arg=$( cygpath --path --ignore --mixed "$arg" )
fi
i=$((i+1))
# Roll the args list around exactly as many times as the number of
# args, so each arg winds up back in the position where it started, but
# possibly modified.
#
# NB: a `for` loop captures its iteration list before it begins, so
# changing the positional parameters here affects neither the number of
# iterations, nor the values presented in `arg`.
shift # remove old arg
set -- "$@" "$arg" # push replacement arg
done
case $i in
(0) set -- ;;
(1) set -- "$args0" ;;
(2) set -- "$args0" "$args1" ;;
(3) set -- "$args0" "$args1" "$args2" ;;
(4) set -- "$args0" "$args1" "$args2" "$args3" ;;
(5) set -- "$args0" "$args1" "$args2" "$args3" "$args4" ;;
(6) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" ;;
(7) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" "$args6" ;;
(8) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" "$args6" "$args7" ;;
(9) set -- "$args0" "$args1" "$args2" "$args3" "$args4" "$args5" "$args6" "$args7" "$args8" ;;
esac
fi

# Escape application args
save () {
for i do printf %s\\n "$i" | sed "s/'/'\\\\''/g;1s/^/'/;\$s/\$/' \\\\/" ; done
echo " "
}
APP_ARGS=$(save "$@")

# Collect all arguments for the java command, following the shell quoting and substitution rules
eval set -- $DEFAULT_JVM_OPTS $JAVA_OPTS $GRADLE_OPTS "\"-Dorg.gradle.appname=$APP_BASE_NAME\"" -classpath "\"$CLASSPATH\"" org.gradle.wrapper.GradleWrapperMain "$APP_ARGS"

# by default we should be in the correct project dir, but when run from Finder on Mac, the cwd is wrong
if [ "$(uname)" = "Darwin" ] && [ "$HOME" = "$PWD" ]; then
cd "$(dirname "$0")"
fi
# Collect all arguments for the java command;
# * $DEFAULT_JVM_OPTS, $JAVA_OPTS, and $GRADLE_OPTS can contain fragments of
# shell script including quotes and variable substitutions, so put them in
# double quotes to make sure that they get re-expanded; and
# * put everything else in single quotes, so that it's not re-expanded.

set -- \
"-Dorg.gradle.appname=$APP_BASE_NAME" \
-classpath "$CLASSPATH" \
org.gradle.wrapper.GradleWrapperMain \
"$@"

# Use "xargs" to parse quoted args.
#
# With -n1 it outputs one arg per line, with the quotes and backslashes removed.
#
# In Bash we could simply go:
#
# readarray ARGS < <( xargs -n1 <<<"$var" ) &&
# set -- "${ARGS[@]}" "$@"
#
# but POSIX shell has neither arrays nor command substitution, so instead we
# post-process each arg (as a line of input to sed) to backslash-escape any
# character that might be a shell metacharacter, then use eval to reverse
# that process (while maintaining the separation between arguments), and wrap
# the whole thing up as a single "set" statement.
#
# This will of course break if any of these variables contains a newline or
# an unmatched quote.
#

eval "set -- $(
printf '%s\n' "$DEFAULT_JVM_OPTS $JAVA_OPTS $GRADLE_OPTS" |
xargs -n1 |
sed ' s~[^-[:alnum:]+,./:=@_]~\\&~g; ' |
tr '\n' ' '
)" '"$@"'

exec "$JAVACMD" "$@"
Loading