Copilot commented on code in PR #50:
URL: https://github.com/apache/solr-orbit/pull/50#discussion_r3658287207
##########
Makefile:
##########
@@ -47,19 +33,15 @@ check-java:
echo "NOTE: Java version 17 required to have all integration tests
pass" >&2; \
fi
-install-deps: check-pip
- $(PIP) install --upgrade pip setuptools wheel
-
-develop: pyinst312 install-deps
- PIP_ONLY_BINARY=h5py $(PIP) install -e .[develop]
+develop: check-uv
+ UV_CONSTRAINT_BINARY=h5py uv sync --extra develop
Review Comment:
`UV_CONSTRAINT_BINARY` is not a known uv configuration knob and will likely
be ignored, meaning `h5py` may be built from source (often failing due to
missing system deps). Use uv’s pip compatibility env var for only-binary
instead.
##########
pyproject.toml:
##########
@@ -19,6 +19,9 @@
requires = ["setuptools>=68", "wheel"]
build-backend = "setuptools.build_meta"
+[tool.uv]
+# uv manages the venv; Python version is controlled by requires-python and
.python-version
+
[project]
Review Comment:
The comment mentions `.python-version`, but the repository does not include
that file; this makes the configuration note misleading. Either add
`.python-version` or adjust the comment to reflect the actual version source.
##########
docker/Dockerfile:
##########
@@ -13,13 +13,15 @@ RUN apt-get -y update && \
apt-get -y upgrade && \
rm -rf /var/lib/apt/lists/*
+# Install uv
+COPY --from=ghcr.io/astral-sh/uv:0.10.2 /uv /uvx /usr/local/bin/
+
WORKDIR /build/solr-orbit
COPY . .
-# There is no binary package currently available for yappi on ARM.
-RUN make build && \
- if test "$(uname -m)" = aarch64; then pip wheel yappi && cp yappi-*.whl
dist; fi
-
+# Build the wheel with uv; also build a yappi wheel on ARM where no binary is
available
+RUN uv build --wheel && \
+ if test "$(uname -m)" = aarch64; then uv pip install --system wheel &&
python -m pip wheel yappi -w dist; fi
Review Comment:
This still invokes `python -m pip ...`, which reintroduces a pip dependency
into the Docker build despite the stated goal of replacing pip with uv. uv can
build wheels directly via `uv pip wheel`.
##########
Makefile:
##########
@@ -16,27 +16,13 @@
# under the License.
SHELL = /bin/bash
-PYTHON = python3
-PIP = pip3
VERSIONS = $(shell jq -r '.python_versions | .[]' .ci/variables.json | sed
'$$d')
Review Comment:
`VERSIONS` is now unused (the pyenv targets were removed), but it still
shells out to `jq`, keeping an unnecessary dependency and slowing `make`
startup. Consider removing it (or replacing it with a comment) to avoid
requiring `jq` for basic targets like `develop`.
##########
run.sh:
##########
@@ -120,18 +106,5 @@ export THESPLOG_THRESHOLD="INFO"
# Provide a consistent binary name to the user and hide the fact that we call
another binary under the hood.
export BENCHMARK_ALTERNATIVE_BINARY_NAME=$(basename "$0")
-if [[ $IN_VIRTUALENV == 0 ]]
-then
- BENCHMARK_ROOT=$(python3 -c "import site; print(site.USER_BASE)")
- BENCHMARK_BIN=${BENCHMARK_ROOT}/bin/${BINARY_NAME}
- install_solrorbit "${BENCHMARK_BIN}"
- if [[ -x $BENCHMARK_BIN ]]; then
- ${BENCHMARK_BIN} "$@"
- else
- echo "Cannot execute ${HUMAN_NAME} in ${BENCHMARK_BIN}."
- fi
-else
- install_solrorbit "${BINARY_NAME}"
-
- ${BINARY_NAME} "$@"
-fi
+install_solrorbit
+uv run ${BINARY_NAME} "$@"
Review Comment:
Unquoted `${BINARY_NAME}` is subject to word-splitting/path-globbing if the
value ever contains whitespace or shell metacharacters. Quoting makes this
robust.
##########
Makefile:
##########
@@ -70,33 +52,28 @@ python-caches-clean:
-@find . -name "__pycache__" -prune -exec rm -rf -- \{\} \;
-@find . -name ".pyc" -prune -exec rm -rf -- \{\} \;
-# Note: pip will not update project dependencies (specified in pyproject.toml)
if any version is
-# already installed; therefore we recommend recreating your environments
whenever your project
-# dependencies change.
tox-env-clean:
rm -rf .tox
-lint:
- ruff check .
- # ruff format --check . # uncomment once the codebase has been
formatted
+lint: develop
+ uv run ruff check .
+ # uv run ruff format --check . # uncomment once the codebase has been
formatted
Review Comment:
Having `lint` depend on `develop` causes an extra `uv sync` in CI (the CI
script already runs `make develop` before `make lint`). You can remove the
dependency and run ruff with the needed extra instead to avoid duplicate
environment work.
This issue also appears on line 62 of the same file.
--
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]
