Copilot commented on code in PR #34: URL: https://github.com/apache/tvm-ffi/pull/34#discussion_r2365423322
########## docs/README.md: ########## @@ -16,31 +16,28 @@ <!--- under the License. --> # TVM FFI Documentation -To build locally +## Build Locally with uv -First install the tvm-ffi package -```bash -pip install .. -``` +All documentation dependencies are managed via [`uv`](https://docs.astral.sh/uv/). +Run the following commands from the repository root: -Install all the requirements to build docs ```bash -pip install -r requirements.txt +uv run --extra docs make -C docs livehtml Review Comment: The README instructs running make under uv, but the Makefile itself already invokes uv internally, causing nested uv calls and redundant environment resolution. Recommend choosing one model: (a) keep uv wrapping in the Makefile and change the README commands to `make -C docs livehtml` / `make -C docs html`, or (b) remove uv usage inside the Makefile (restore SPHINXBUILD and livehtml to plain python/sphinx commands) and keep the current README commands. Clarifying this avoids confusion and unnecessary overhead. ########## docs/Makefile: ########## @@ -16,8 +16,9 @@ # under the License. # You can set these variables from the command line, and also # from the environment for the first two. +UV_RUN ?= uv run --extra docs Review Comment: Including uv invocation inside both SPHINXBUILD and the livehtml target leads to nested uv execution when users follow the current README (which already wraps make with uv). This duplication can slow iterative builds and obscures responsibility for environment setup. Either remove $(UV_RUN) here (expecting callers to use uv) or keep it and update the README to call plain make; consolidating responsibility in one layer will simplify maintenance. ########## docs/README.md: ########## @@ -16,31 +16,28 @@ <!--- under the License. --> # TVM FFI Documentation -To build locally +## Build Locally with uv -First install the tvm-ffi package -```bash -pip install .. -``` +All documentation dependencies are managed via [`uv`](https://docs.astral.sh/uv/). +Run the following commands from the repository root: -Install all the requirements to build docs ```bash -pip install -r requirements.txt +uv run --extra docs make -C docs livehtml ``` -Then build the doc -```bash -make livehtml -``` +For a one-off build you can run `uv run --extra docs make -C docs html` instead. Review Comment: The README instructs running make under uv, but the Makefile itself already invokes uv internally, causing nested uv calls and redundant environment resolution. Recommend choosing one model: (a) keep uv wrapping in the Makefile and change the README commands to `make -C docs livehtml` / `make -C docs html`, or (b) remove uv usage inside the Makefile (restore SPHINXBUILD and livehtml to plain python/sphinx commands) and keep the current README commands. Clarifying this avoids confusion and unnecessary overhead. ########## docs/Makefile: ########## @@ -28,7 +29,7 @@ help: .PHONY: help Makefile livehtml clean livehtml: - @sphinx-autobuild "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O) --ignore reference/cpp/generated + @$(UV_RUN) sphinx-autobuild "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O) --ignore reference/cpp/generated Review Comment: Including uv invocation inside both SPHINXBUILD and the livehtml target leads to nested uv execution when users follow the current README (which already wraps make with uv). This duplication can slow iterative builds and obscures responsibility for environment setup. Either remove $(UV_RUN) here (expecting callers to use uv) or keep it and update the README to call plain make; consolidating responsibility in one layer will simplify maintenance. ```suggestion @sphinx-autobuild "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O) --ignore reference/cpp/generated ``` ########## docs/Makefile: ########## @@ -16,8 +16,9 @@ # under the License. # You can set these variables from the command line, and also # from the environment for the first two. +UV_RUN ?= uv run --extra docs SPHINXOPTS ?= -SPHINXBUILD ?= python3 -m sphinx +SPHINXBUILD ?= $(UV_RUN) python -m sphinx Review Comment: Including uv invocation inside both SPHINXBUILD and the livehtml target leads to nested uv execution when users follow the current README (which already wraps make with uv). This duplication can slow iterative builds and obscures responsibility for environment setup. Either remove $(UV_RUN) here (expecting callers to use uv) or keep it and update the README to call plain make; consolidating responsibility in one layer will simplify maintenance. ```suggestion SPHINXBUILD ?= python -m sphinx ``` -- 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]
