Skip to content

Conversation

ykdojo
Copy link
Contributor

@ykdojo ykdojo commented Aug 27, 2025

Changes Made

Fixed missing source command in the install-docs-deps target that was causing "Permission denied" errors when running make docs.

Problem

The install-docs-deps target was trying to execute the activate script directly instead of sourcing it:

$(VENV_BIN)/activate && uv sync --all-extras --all-groups

This resulted in permission denied errors because the shell was attempting to execute the script rather than source it into the current environment.

Solution

Added the missing source command to be consistent with other targets in the Makefile:

source $(VENV_BIN)/activate && uv sync --all-extras --all-groups
source $(VENV_BIN)/activate && uv pip install -e docs/plugins/nav_hide_children

This change makes the install-docs-deps target consistent with other targets like hooks, check-format, lint, etc. that correctly use source $(VENV_BIN)/activate.

The install-docs-deps target was missing the 'source' command before
$(VENV_BIN)/activate, causing "Permission denied" errors when running
make docs. This makes it consistent with other targets in the Makefile
that correctly use 'source' to activate the virtual environment.
@github-actions github-actions bot added the fix label Aug 27, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR fixes a critical shell scripting error in the Makefile's install-docs-deps target. The issue was that the target was attempting to execute the virtual environment activation script directly ($(VENV_BIN)/activate) instead of sourcing it into the current shell environment. In Unix/Linux shell scripting, virtual environment activation scripts must be sourced (not executed) to properly modify the current shell's environment variables and PATH.

The fix adds the missing source command before the activation script path, changing:

$(VENV_BIN)/activate && uv sync --all-extras --all-groups

To:

source $(VENV_BIN)/activate && uv sync --all-extras --all-groups
source $(VENV_BIN)/activate && uv pip install -e docs/plugins/nav_hide_children

This change brings the install-docs-deps target into consistency with other Makefile targets like hooks, check-format, and lint that already correctly use source $(VENV_BIN)/activate. The Makefile is a central build automation tool in the Daft repository that handles various development tasks including building, testing, and documentation generation. This fix ensures that the documentation build pipeline (make docs) works correctly by resolving the dependency installation step that was previously failing with "Permission denied" errors.

Confidence score: 5/5

  • This PR is extremely safe to merge with no risk of breaking anything
  • Score reflects a simple, well-understood shell scripting fix that resolves a clear functional issue
  • No files require special attention as this is a straightforward syntax correction

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

Copy link

codecov bot commented Aug 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.25%. Comparing base (8126542) to head (6ca4d17).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5060      +/-   ##
==========================================
- Coverage   76.55%   75.25%   -1.31%     
==========================================
  Files         947      948       +1     
  Lines      129874   133054    +3180     
==========================================
+ Hits        99430   100131     +701     
- Misses      30444    32923    +2479     

see 66 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ykdojo ykdojo merged commit 7d19c83 into main Aug 27, 2025
53 of 54 checks passed
@ykdojo ykdojo deleted the fix/makefile-source-command branch August 27, 2025 20:21
Jay-ju pushed a commit to Jay-ju/Daft that referenced this pull request Sep 1, 2025
…ventual-Inc#5060)

## Changes Made

Fixed missing `source` command in the `install-docs-deps` target that
was causing "Permission denied" errors when running `make docs`.

## Problem

The `install-docs-deps` target was trying to execute the activate script
directly instead of sourcing it:
```makefile
$(VENV_BIN)/activate && uv sync --all-extras --all-groups
```

This resulted in permission denied errors because the shell was
attempting to execute the script rather than source it into the current
environment.

## Solution

Added the missing `source` command to be consistent with other targets
in the Makefile:
```makefile
source $(VENV_BIN)/activate && uv sync --all-extras --all-groups
source $(VENV_BIN)/activate && uv pip install -e docs/plugins/nav_hide_children
```

This change makes the `install-docs-deps` target consistent with other
targets like `hooks`, `check-format`, `lint`, etc. that correctly use
`source $(VENV_BIN)/activate`.
venkateshdb pushed a commit to venkateshdb/Daft that referenced this pull request Sep 6, 2025
…ventual-Inc#5060)

## Changes Made

Fixed missing `source` command in the `install-docs-deps` target that
was causing "Permission denied" errors when running `make docs`.

## Problem

The `install-docs-deps` target was trying to execute the activate script
directly instead of sourcing it:
```makefile
$(VENV_BIN)/activate && uv sync --all-extras --all-groups
```

This resulted in permission denied errors because the shell was
attempting to execute the script rather than source it into the current
environment.

## Solution

Added the missing `source` command to be consistent with other targets
in the Makefile:
```makefile
source $(VENV_BIN)/activate && uv sync --all-extras --all-groups
source $(VENV_BIN)/activate && uv pip install -e docs/plugins/nav_hide_children
```

This change makes the `install-docs-deps` target consistent with other
targets like `hooks`, `check-format`, `lint`, etc. that correctly use
`source $(VENV_BIN)/activate`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant