Copilot commented on code in PR #19863: URL: https://github.com/apache/datafusion/pull/19863#discussion_r2700701635
########## docs/Dockerfile: ########## @@ -0,0 +1,58 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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 +# +# http://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. + +FROM python:3.11-slim + +# Install system dependencies +RUN apt-get update && apt-get install -y --no-install-recommends \ + curl \ + build-essential \ + graphviz \ + && rm -rf /var/lib/apt/lists/* + +# Install Rust 1.92.0 +ENV RUST_VERSION=1.92.0 +RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- \ + --default-toolchain ${RUST_VERSION} \ + --component rustfmt \ + --profile minimal \ + -y + +ENV PATH="/root/.cargo/bin:${PATH}" + +# Install cargo-depgraph +RUN cargo install cargo-depgraph --version ^1.6 --locked + +# Set working directory +WORKDIR /work + +# Copy Python requirements and install them +COPY docs/requirements.txt /work/docs/requirements.txt +RUN pip install --no-cache-dir -r /work/docs/requirements.txt + +# Copy the entire repository +COPY . /work + +# Fix line endings for shell scripts (convert CRLF to LF) +RUN find /work -name "*.sh" -type f -exec sh -c 'tr -d "\r" < "$1" > "$1.tmp" && mv "$1.tmp" "$1"' _ {} \; || true Review Comment: The `|| true` at the end suppresses all errors from the line ending conversion, which could hide legitimate failures. If a script file is critical for the build and fails to convert, the build should fail rather than continue silently. Consider removing the `|| true` to allow proper error propagation, or at minimum add logging to indicate when conversion fails. ```suggestion RUN find /work -name "*.sh" -type f -exec sh -c 'tr -d "\r" < "$1" > "$1.tmp" && mv "$1.tmp" "$1"' _ {} \; ``` ########## docs/README.md: ########## @@ -25,11 +25,29 @@ https://datafusion.apache.org/ as part of the release process. ## Dependencies +### Option 1: Docker (Recommended) + +If you have Docker installed, you can build the docs without installing any dependencies on your system: + +```sh +# Using docker-compose (simplest) +docker-compose run --rm docs bash build.sh + +# Or using docker directly +docker build -t datafusion-docs -f docs/Dockerfile . +docker run --rm -v $(pwd):/work datafusion-docs bash build.sh Review Comment: The volume mount syntax `$(pwd)` is shell-specific and won't work in PowerShell on Windows. Consider adding a note about Windows PowerShell users needing to use `${PWD}` instead, or provide separate examples for different shells to ensure cross-platform compatibility as claimed in the PR description. ```suggestion # Using docker-compose (simplest) (POSIX shells: bash, zsh, etc.) docker-compose run --rm docs bash build.sh # Or using docker directly (POSIX shells: bash, zsh, etc.) docker build -t datafusion-docs -f docs/Dockerfile . docker run --rm -v $(pwd):/work datafusion-docs bash build.sh # On Windows PowerShell, use: # docker run --rm -v ${PWD}:/work datafusion-docs bash build.sh ``` ########## docs/Dockerfile: ########## @@ -0,0 +1,58 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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 +# +# http://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. + +FROM python:3.11-slim + +# Install system dependencies +RUN apt-get update && apt-get install -y --no-install-recommends \ + curl \ + build-essential \ + graphviz \ + && rm -rf /var/lib/apt/lists/* + +# Install Rust 1.92.0 +ENV RUST_VERSION=1.92.0 +RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- \ + --default-toolchain ${RUST_VERSION} \ + --component rustfmt \ + --profile minimal \ + -y Review Comment: The Rust installation uses a pipe from curl directly to sh, which could pose security risks if the connection is compromised. While the `--proto '=https'` and `--tlsv1.2` flags provide some protection, consider adding checksum verification of the downloaded installer script or using a specific version/commit of the rustup-init script for better reproducibility and security. ```suggestion ENV RUSTUP_VERSION=1.27.1 RUN set -eux; \ arch="$(uname -m)"; \ case "${arch}" in \ x86_64) rustup_arch="x86_64-unknown-linux-gnu" ;; \ aarch64) rustup_arch="aarch64-unknown-linux-gnu" ;; \ *) echo "Unsupported architecture: ${arch}" >&2; exit 1 ;; \ esac; \ curl --proto '=https' --tlsv1.2 -sSf \ "https://static.rust-lang.org/rustup/archive/${RUSTUP_VERSION}/${rustup_arch}/rustup-init"; \ -o rustup-init; \ chmod +x rustup-init; \ ./rustup-init \ --default-toolchain "${RUST_VERSION}" \ --component rustfmt \ --profile minimal \ -y; \ rm rustup-init ``` ########## docs/Dockerfile: ########## @@ -0,0 +1,58 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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 +# +# http://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. + +FROM python:3.11-slim + +# Install system dependencies +RUN apt-get update && apt-get install -y --no-install-recommends \ + curl \ + build-essential \ + graphviz \ + && rm -rf /var/lib/apt/lists/* + +# Install Rust 1.92.0 +ENV RUST_VERSION=1.92.0 +RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- \ + --default-toolchain ${RUST_VERSION} \ + --component rustfmt \ + --profile minimal \ + -y + +ENV PATH="/root/.cargo/bin:${PATH}" + +# Install cargo-depgraph +RUN cargo install cargo-depgraph --version ^1.6 --locked + +# Set working directory +WORKDIR /work + +# Copy Python requirements and install them +COPY docs/requirements.txt /work/docs/requirements.txt +RUN pip install --no-cache-dir -r /work/docs/requirements.txt + +# Copy the entire repository +COPY . /work + +# Fix line endings for shell scripts (convert CRLF to LF) +RUN find /work -name "*.sh" -type f -exec sh -c 'tr -d "\r" < "$1" > "$1.tmp" && mv "$1.tmp" "$1"' _ {} \; || true + +# Set working directory to docs for build +WORKDIR /work/docs + +# Default command: build documentation +# Fix line endings when running (for volume-mounted files from Windows) and then build +CMD ["bash", "-c", "find /work -name '*.sh' -type f -exec sh -c 'tr -d \"\\r\" < \"$1\" > \"$1.tmp\" && mv \"$1.tmp\" \"$1\"' _ {} \\; ; cd /work/docs && bash build.sh"] Review Comment: The CMD duplicates the line ending fix that was already performed at build time (line 51). When using volume mounts, the build-time fix won't affect mounted files, but this approach is inefficient because it runs on every container start. Consider documenting this behavior or using a more efficient approach, such as checking if line ending conversion is needed before running it, or only running it when volumes are detected. ```suggestion # Fix line endings when running (for volume-mounted files from Windows) only if needed, and then build CMD ["bash", "-c", "if find /work -name '*.sh' -type f -print0 | xargs -0 grep -Il $'\\r' >/dev/null 2>&1; then echo \"Converting CRLF to LF in shell scripts...\"; find /work -name '*.sh' -type f -exec sh -c 'tr -d \"\\r\" < \"$1\" > \"$1.tmp\" && mv \"$1.tmp\" \"$1\"' _ {} \\; ; fi; cd /work/docs && bash build.sh"] ``` ########## docker-compose.yml: ########## @@ -0,0 +1,27 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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 +# +# http://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. + +version: '3.8' + Review Comment: The version '3.8' of Docker Compose is from the legacy format. Docker Compose v2 (the current version) doesn't require a version field, and the Compose Specification recommends omitting it. Consider removing the version field to follow current best practices, as it's ignored by modern Docker Compose versions anyway. ```suggestion ``` ########## docs/Dockerfile: ########## @@ -0,0 +1,58 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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 +# +# http://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. + +FROM python:3.11-slim + +# Install system dependencies +RUN apt-get update && apt-get install -y --no-install-recommends \ + curl \ + build-essential \ + graphviz \ + && rm -rf /var/lib/apt/lists/* + +# Install Rust 1.92.0 +ENV RUST_VERSION=1.92.0 +RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- \ + --default-toolchain ${RUST_VERSION} \ + --component rustfmt \ + --profile minimal \ + -y + +ENV PATH="/root/.cargo/bin:${PATH}" + +# Install cargo-depgraph +RUN cargo install cargo-depgraph --version ^1.6 --locked + +# Set working directory +WORKDIR /work + +# Copy Python requirements and install them +COPY docs/requirements.txt /work/docs/requirements.txt +RUN pip install --no-cache-dir -r /work/docs/requirements.txt + +# Copy the entire repository +COPY . /work + +# Fix line endings for shell scripts (convert CRLF to LF) +RUN find /work -name "*.sh" -type f -exec sh -c 'tr -d "\r" < "$1" > "$1.tmp" && mv "$1.tmp" "$1"' _ {} \; || true + +# Set working directory to docs for build +WORKDIR /work/docs + +# Default command: build documentation +# Fix line endings when running (for volume-mounted files from Windows) and then build +CMD ["bash", "-c", "find /work -name '*.sh' -type f -exec sh -c 'tr -d \"\\r\" < \"$1\" > \"$1.tmp\" && mv \"$1.tmp\" \"$1\"' _ {} \\; ; cd /work/docs && bash build.sh"] Review Comment: The escaped backslash and quotes in the CMD make this command difficult to read and maintain. Consider extracting this logic into a separate shell script (e.g., `entrypoint.sh`) that can be copied into the image and executed directly. This would improve readability and make the logic easier to test and modify. ```suggestion RUN cat << 'EOF' > /usr/local/bin/entrypoint.sh #!/usr/bin/env bash set -euo pipefail # Fix line endings when running (for volume-mounted files from Windows) find /work -name '*.sh' -type f -exec sh -c 'tr -d "\r" < "$1" > "$1.tmp" && mv "$1.tmp" "$1"' _ {} \; # Build documentation cd /work/docs bash build.sh EOF RUN chmod +x /usr/local/bin/entrypoint.sh CMD ["bash", "/usr/local/bin/entrypoint.sh"] ``` -- 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]
