Copilot commented on code in PR #37302:
URL: https://github.com/apache/superset/pull/37302#discussion_r2713934146
##########
docker/apt-install.sh:
##########
@@ -38,9 +38,16 @@ RESET='\033[0m'
echo -e "${GREEN}Updating package lists...${RESET}"
apt-get update -qq
+# Ensure native Python extensions can compile (python-geohash, psycopg2,
mysqlclient)
+echo -e "${GREEN}Installing build toolchain...${RESET}"
+apt-get install -yqq --no-install-recommends \
+ build-essential \
+ g++
+
echo -e "${GREEN}Installing packages: $@${RESET}"
apt-get install -yqq --no-install-recommends "$@"
+
Review Comment:
Installing build-essential permanently in apt-install.sh conflicts with the
existing strategy in pip-install.sh. The pip-install.sh script (lines 41-44,
56-62) temporarily installs build-essential when needed and then removes it to
keep the final image lean. By installing build-essential permanently here,
you're bypassing this optimization and significantly increasing the final image
size.
Instead, consider ensuring pip-install.sh is called with the
--requires-build-essential flag when installing packages that need native
compilation, or verify that the base.txt installation already uses this flag
(which it does at line 245 in Dockerfile).
```suggestion
echo -e "${GREEN}Installing packages: $@${RESET}"
apt-get install -yqq --no-install-recommends "$@"
```
##########
docker/apt-install.sh:
##########
@@ -38,9 +38,16 @@ RESET='\033[0m'
echo -e "${GREEN}Updating package lists...${RESET}"
apt-get update -qq
+# Ensure native Python extensions can compile (python-geohash, psycopg2,
mysqlclient)
+echo -e "${GREEN}Installing build toolchain...${RESET}"
+apt-get install -yqq --no-install-recommends \
+ build-essential \
+ g++
Review Comment:
The `build-essential` package already includes `g++` as a dependency, so
explicitly listing `g++` here is redundant. Consider removing the redundant
`g++` line to keep the package list concise.
```suggestion
build-essential
```
##########
docker/apt-install.sh:
##########
@@ -38,9 +38,16 @@ RESET='\033[0m'
echo -e "${GREEN}Updating package lists...${RESET}"
apt-get update -qq
+# Ensure native Python extensions can compile (python-geohash, psycopg2,
mysqlclient)
+echo -e "${GREEN}Installing build toolchain...${RESET}"
+apt-get install -yqq --no-install-recommends \
+ build-essential \
+ g++
+
echo -e "${GREEN}Installing packages: $@${RESET}"
apt-get install -yqq --no-install-recommends "$@"
+
Review Comment:
This blank line adds unnecessary whitespace. Consider removing it to
maintain consistent formatting with the rest of the script.
```suggestion
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
