[Maint] add Makefile to replace format.sh
#1083
Conversation
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
WalkthroughAdds a comprehensive Makefile providing targets for installation, editable installs, uninstall, formatting, linting, testing, C/C++ build, docs, and cleanup. Updates CONTRIBUTING.md to reference Make targets and local docs serving. Adds CMake ignores to .gitignore, refines Ruff excludes in pyproject.toml, and revises MANIFEST.in packaging entries and prunes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Make as make
participant Shell as Shell/Env
participant Py as Python tools
participant Cpp as C/C++ tools
participant Docs as Sphinx
Dev->>Make: make <target> (install / install-editable / test / format / docs / cmake-build)
Make->>Shell: set flags, check PATH, read project metadata
alt Python targets
Make->>Py: check/install pip deps, pre-commit, yapf, ruff, pytest
Py-->>Make: run formatting/lint/tests -> results
else C/C++ targets
Make->>Cpp: check/install cmake, clang-format, clang-tidy
Cpp-->>Make: configure/build/format/lint -> results
else Docs
Make->>Py: install docs deps
Make->>Docs: sphinx-build / sphinx-autobuild (watch & serve)
Docs-->>Make: serve URL/logs
end
Make-->>Dev: exit code & logs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
XuehaiPan
force-pushed
the
add-makefile
branch
from
9d8313d to
a617393
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Makefile (1)
166-167: Add the standardalltarget.Static analysis tools and Makefile conventions expect an
allphony target. While the currentdefaulttarget provides similar functionality, addingallas an alias or primary target improves convention compliance.Consider adding the
alltarget:+.PHONY: all +all: default + .PHONY: default default: installAlternatively, rename
defaulttoallif it better reflects the intended primary workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 166-166: Missing required phony target "all"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
- GitHub Check: Test for Python 3.12 with ROCm-6.3 (on self-hosted-amd)
XuehaiPan
force-pushed
the
add-makefile
branch
from
a617393 to
17ece1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Makefile (2)
35-35: Removecd &&from thecheck_pip_installhelper—this issue was flagged in review but remains unfixed.The helper invokes
cd &&which changes to$HOMEbefore installing packages. This is problematic in CI/CD environments or workspaces with specific isolation requirements, and it's unnecessary for package installation. Packages install correctly in the current environment.Apply this diff to remove the directory change:
-check_pip_install = $(PYTHON) -m pip show $(1) &>/dev/null || (cd && $(PYTHON) -m pip install --upgrade $(1)) +check_pip_install = $(PYTHON) -m pip show $(1) &>/dev/null || $(PYTHON) -m pip install --upgrade $(1)
150-151: Separatelintandformatconcerns—this issue was flagged in review but remains unfixed.The
linttarget depends onformat, which meansmake lintautomatically reformats code before linting. This violates separation of concerns: linting should check code without modifying it. Users may not expect their code to be changed by a lint target. Keepformatfor modifications andlintfor checks only.Consider restructuring:
.PHONY: format format: pre-commit python-format ruff clang-format .PHONY: lint -lint: format clang-tidy +lint: pre-commit python-format ruff clang-format clang-tidyAlternatively, rename a combined target to
format-and-lintto signal both operations occur.
🧹 Nitpick comments (1)
Makefile (1)
16-17: Add anallphony target per Makefile conventions.Static analysis tools expect an
alltarget as the canonical default. Whiledefaultworks, consider addingallfor broader compatibility:+.PHONY: all +all: install + .PHONY: default default: installAlternatively, set
.DEFAULT_GOAL := installat the top of the file to specify the implicit default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 166-166: Missing required phony target "all"
(minphony)
🔇 Additional comments (2)
Makefile (2)
1-17: Project metadata and defaults are well-structured.The shell configuration with strict error handling (
-eu -o pipefail) and comprehensive file discovery patterns (Python, C++, CUDA) provide a solid foundation. The explicit inclusion ofversion_provider.pyat the root is a good defensive measure.
91-95: LGTM – yapf configuration is correct.The flags
--in-placeand--parallelare complementary and appropriate. The pre-check fordocs/conf.pyhandles a special case gracefully.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Makefile (2)
35-35: Unresolved: Removecd &&from thecheck_pip_installhelper function.This was previously flagged but remains unaddressed. The helper invokes
cd &&which changes to the home directory before installing packages. This is problematic in CI/CD environments or isolated workspaces—packages should install in the current environment regardless of working directory.Apply this diff to fix:
-check_pip_install = $(PYTHON) -m pip show $(1) &>/dev/null || (cd && $(PYTHON) -m pip install --upgrade $(1)) +check_pip_install = $(PYTHON) -m pip show $(1) &>/dev/null || $(PYTHON) -m pip install --upgrade $(1)
152-153: Unresolved: Separatelintandformattargets.This was previously flagged but remains unaddressed. The
linttarget depends onformat, causingmake lintto automatically reformat code. This violates separation of concerns—linting should check code without modifying it. Users may not expect their code to be modified by a lint target.Apply this diff to fix:
.PHONY: format format: pre-commit python-format ruff clang-format .PHONY: lint -lint: format clang-tidy +lint: pre-commit python-format ruff clang-format clang-tidy
🧹 Nitpick comments (1)
Makefile (1)
1-5: Add the.PHONY: alltarget.Per Makefile best practices, an explicit
alltarget should be defined and marked as phony. Currently,defaultpoints toinstall, but standard convention reservesallas the primary entry point.Add this target definition near the top of the file after the metadata:
.PHONY: default default: install + +.PHONY: all +all: default
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 168-168: Missing required phony target "all"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test for Python 3.12 with ROCm-6.3 (on self-hosted-amd)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
99-99: Update "Test Locally" section for consistency.The "Install Develop Version" and "Lint Check" sections now reference Make targets, but the "Test Locally" section (line 99) still uses a direct
python3 -m pytestcommand. Per the PR objectives,make testis a supported target.For consistency, update line 99 to reference the Make target:
-Then you can rerun the tests with: +Then you can rerun the tests with: -```bash -python3 -m pytest testing -``` +```bash +make test +``` + +You can also pass pytest options: + +```bash +make test PYTESTOPTS="--maxfail=3 --numprocesses=4" +```This also exposes users to the flexibility of passing pytest options through the Makefile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CONTRIBUTING.md(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
- GitHub Check: Test for Python 3.12 with ROCm-6.3 (on self-hosted-amd)
🔇 Additional comments (2)
CONTRIBUTING.md (2)
75-75: Make targets appropriately integrated. Good update to referencemake install-editableandmake lintinstead of raw commands.Also applies to: 89-89
108-114: Documentation section formake docsclearly describes the watch behavior. Good addition explaining the auto-rebuild and browser refresh capability, plus the localhost URL.Verify that the Makefile's
docstarget actually implements the watch and auto-rebuild behavior described in the documentation.
|
would also be great for developers to checkout this modifications, cc @chengyupku @Rachmanino @tzj-fxz @kurisu6912 @Elevator14B |
2 participants
Resolves #1044 (comment)
Example usages:
cc @LeiWang1999
Summary by CodeRabbit
Chores
Documentation