-
Notifications
You must be signed in to change notification settings - Fork 427
infra: allow rerun make install without prompt
#2979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
rambleraptor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fantastic change! uv sync can be a very costly operation.
...especially if you're on a containerized environment where you might have a timeout to stand up a service (yes, I ran into a situation on a different project an hour ago)
20ceaf7 to
d017475
Compare
| uv sync $(PYTHON_ARG) --all-extras | ||
| @# Reinstall pyiceberg if Cython extensions (.so) are missing after `make clean` (see #2869) | ||
| @if ! find pyiceberg -name "*.so" 2>/dev/null | grep -q .; then \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since uv sync is super fast, its fine to call it (potentially) twice
| install-hooks: ## Install pre-commit hooks | ||
| install: install-uv ## Install uv, dependencies, and pre-commit hooks | ||
| uv sync $(PYTHON_ARG) --all-extras | ||
| @# Reinstall pyiceberg if Cython extensions (.so) are missing after `make clean` (see #2869) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@# is the syntax for comment. Using just # will have the sentence echo out when running make install
Rationale for this change
This PR lets
uvmanage the virtual env. uv will only setup a new venv if it does not exist and sync dep only when necessary.This should make the entire
make installprocess a lot faster and easier to work withContext
I noticed running
make installwhen a.venvalready existed would show an interactive prompt:We dont need this prompt. And more crucially, claude keeps on getting stuck on this prompt 😞
Are these changes tested?
Yes running
make installrepeatedlyAre there any user-facing changes?