rnewson commented on code in PR #4871:
URL: https://github.com/apache/couchdb/pull/4871#discussion_r1407474905
##########
Makefile.win:
##########
@@ -241,13 +241,20 @@ elixir: elixir-init devclean
--erlang-config rel/files/eunit.config \
--no-eval 'mix test --trace --include
test\elixir\test\config\suite.elixir --exclude
test\elixir\test\config\skip.elixir $(EXUNIT_OPTS)'
+ifneq ($(CLOUSEAU_DIR),)
+_WITH_CLOUSEAU="--with-clouseau --clouseau-dir=$(CLOUSEAU_DIR)"
+else ifeq ($(with_clouseau), 1)
+_WITH_CLOUSEAU=--with-clouseau
Review Comment:
quotes around value (readability)
##########
Makefile:
##########
@@ -263,13 +263,20 @@ elixir: elixir-init devclean
--erlang-config rel/files/eunit.config \
--no-eval 'mix test --trace --include
test/elixir/test/config/suite.elixir --exclude
test/elixir/test/config/skip.elixir $(EXUNIT_OPTS)'
+ifneq ($(CLOUSEAU_DIR),)
+_WITH_CLOUSEAU="--with-clouseau --clouseau-dir=$(CLOUSEAU_DIR)"
+else ifeq ($(with_clouseau), 1)
+_WITH_CLOUSEAU=--with-clouseau
Review Comment:
quotes around value (readability)
##########
configure:
##########
@@ -381,32 +421,46 @@ install_local_erlfmt() {
}
install_local_clouseau() {
-
_DIST_URL=https://github.com/cloudant-labs/clouseau/releases/download/"$CLOUSEAU_VSN"/clouseau-"$CLOUSEAU_VSN"-dist.zip
- _MAVEN_BASE_URI=https://repo1.maven.org/maven2
-
- _SLF4J_SIMPLE_VSN=${SLF4J_SIMPLE_VERSION:-1.7.36}
- _SLF4J_SIMPLE_JAR=slf4j-simple-"$_SLF4J_SIMPLE_VSN".jar
-
_SLF4J_SIMPLE_URL="$_MAVEN_BASE_URI"/org/slf4j/slf4j-simple/"$_SLF4J_SIMPLE_VSN"/"$_SLF4J_SIMPLE_JAR"
-
- rm -rf "$CLOUSEAU_DIR"
- mkdir -p "$CLOUSEAU_DIR"
-
- if ! curl -sSL --max-redirs 1 -o clouseau.zip "$_DIST_URL"; then
- printf "ERROR: %s could not be downloaded.\n" "$_DIST_URL" >&2
- exit 1
- fi
-
- if ! unzip -q -j clouseau.zip -d "$CLOUSEAU_DIR"; then
- printf "ERROR: Clouseau distribution package (clouseau.zip) could not
be extracted.\n" >&2
- exit 1
- fi
-
- rm clouseau.zip
-
- if ! curl -sSL --max-redirs 1 -o "$CLOUSEAU_DIR"/"$_SLF4J_SIMPLE_JAR"
"$_SLF4J_SIMPLE_URL"; then
- printf "ERROR: %s could not be downloaded.\n" "$_SLF4J_SIMPLE_URL" >&2
- exit 1
- fi
+ case "$CLOUSEAU_MTH" in
+ [Dd][Ii][Ss][Tt])
+ _DIST_URL=$(printf "$CLOUSEAU_URI" "$CLOUSEAU_VSN" "$CLOUSEAU_VSN")
+ _MAVEN_BASE_URI=https://repo1.maven.org/maven2
+
+ _SLF4J_SIMPLE_VSN=${SLF4J_SIMPLE_VERSION:-1.7.36}
+ _SLF4J_SIMPLE_JAR=slf4j-simple-"$_SLF4J_SIMPLE_VSN".jar
+
_SLF4J_SIMPLE_URL="$_MAVEN_BASE_URI"/org/slf4j/slf4j-simple/"$_SLF4J_SIMPLE_VSN"/"$_SLF4J_SIMPLE_JAR"
+
+ rm -rf "$CLOUSEAU_DIR"
+ mkdir -p "$CLOUSEAU_DIR"
+
+ echo "Fetching Clouseau from $_DIST_URL..."
+ if ! curl -sSL --max-redirs 1 -o clouseau.zip "$_DIST_URL"; then
+ printf "ERROR: %s could not be downloaded.\n" "$_DIST_URL" >&2
+ exit 1
+ fi
+
+ if ! unzip -q -j clouseau.zip -d "$CLOUSEAU_DIR"; then
+ printf "ERROR: Clouseau distribution package (clouseau.zip)
could not be extracted.\n" >&2
+ exit 1
+ fi
+
+ rm clouseau.zip
+
+ if ! curl -sSL --max-redirs 1 -o
"$CLOUSEAU_DIR"/"$_SLF4J_SIMPLE_JAR" "$_SLF4J_SIMPLE_URL"; then
+ printf "ERROR: %s could not be downloaded.\n"
"$_SLF4J_SIMPLE_URL" >&2
+ exit 1
+ fi
+ ;;
+
+ [Gg][Ii][Tt])
+ echo "Cloning Clouseau from $CLOUSEAU_URI ($CLOUSEAU_VSN)..."
+ rm -rf "$CLOUSEAU_DIR"
Review Comment:
A forced recursive rm here on a variable is potentially very dangerous. I
don't think it's acceptable in dev/run.
Instead you could test for the existence of the folder and bail if it exists
and isn't already a clone of the clouseau repo.
##########
README-DEV.rst:
##########
@@ -317,6 +317,32 @@ used to configure Clouseau to work with that::
dev/run --with-clouseau --erlang-cookie=brumbrum
+To facilitate a smoother development flow, it is possible to override
Review Comment:
Wordy description that isn't imo useful to the reader. Can we tighten the
text here to what these options do and why they might be useful? Striking
everything before "it" in the first sentence is the kind of edit I'm thinking
of.
##########
configure:
##########
@@ -381,32 +421,46 @@ install_local_erlfmt() {
}
install_local_clouseau() {
-
_DIST_URL=https://github.com/cloudant-labs/clouseau/releases/download/"$CLOUSEAU_VSN"/clouseau-"$CLOUSEAU_VSN"-dist.zip
- _MAVEN_BASE_URI=https://repo1.maven.org/maven2
-
- _SLF4J_SIMPLE_VSN=${SLF4J_SIMPLE_VERSION:-1.7.36}
- _SLF4J_SIMPLE_JAR=slf4j-simple-"$_SLF4J_SIMPLE_VSN".jar
-
_SLF4J_SIMPLE_URL="$_MAVEN_BASE_URI"/org/slf4j/slf4j-simple/"$_SLF4J_SIMPLE_VSN"/"$_SLF4J_SIMPLE_JAR"
-
- rm -rf "$CLOUSEAU_DIR"
- mkdir -p "$CLOUSEAU_DIR"
-
- if ! curl -sSL --max-redirs 1 -o clouseau.zip "$_DIST_URL"; then
- printf "ERROR: %s could not be downloaded.\n" "$_DIST_URL" >&2
- exit 1
- fi
-
- if ! unzip -q -j clouseau.zip -d "$CLOUSEAU_DIR"; then
- printf "ERROR: Clouseau distribution package (clouseau.zip) could not
be extracted.\n" >&2
- exit 1
- fi
-
- rm clouseau.zip
-
- if ! curl -sSL --max-redirs 1 -o "$CLOUSEAU_DIR"/"$_SLF4J_SIMPLE_JAR"
"$_SLF4J_SIMPLE_URL"; then
- printf "ERROR: %s could not be downloaded.\n" "$_SLF4J_SIMPLE_URL" >&2
- exit 1
- fi
+ case "$CLOUSEAU_MTH" in
+ [Dd][Ii][Ss][Tt])
Review Comment:
just accept `dist` and `git` (readability of code and not tolerating bad
input). exit with an error for any other value than the two documented as
correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
