[fix] Allow easily building docs with local changes again #251 #257
Conversation
📝 WalkthroughWalkthroughThe changes add a local-development pathway to build.py: when PRODUCTION is unset, the script creates a staging directory, symlinks non-excluded top-level items from the local working tree (skipping staging-dir, modules, _build, .git, pycache and pyvenv-based virtualenvs detected via pyvenv.cfg), and exits early to use that local setup. For production runs, repository update/checkout fetches the remote branch and checks out Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (local working tree)
participant Build as build.py
participant FS as Filesystem (staging-dir)
Dev->>Build: invoke build (PRODUCTION unset)
Build->>FS: create staging-dir
Build->>Dev: list top-level items
alt item is top-level and not excluded and not pyvenv venv
Build->>FS: create symlink to item
else excluded or pyvenv venv
Build-->>FS: skip item
end
Build->>Build: exit early (use local staging-dir)
sequenceDiagram
participant Build as build.py
participant Git as Git / Remote
participant Repo as local clone dir
Build->>Git: clone via SSH/HTTPS or reuse existing clone
Build->>Repo: git fetch <branch>
Build->>Repo: git -c advice.detachedHead=false checkout refs/heads/{branch}
Repo-->>Build: branch checked out
Build->>Build: proceed with build using checked-out branch
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.14.13)build.py317-322: Starting a process with a partial executable path (S607) 328-328: (S603) 329-337: Starting a process with a partial executable path (S607) ⏰ 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). (1)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@build.py`:
- Around line 275-298: The staging-dir handling in the openwisp-docs local-build
branch must fully reset staging-dir and honor the docs/ convention used by
clone_or_update_repo(): before creating anything, if "staging-dir" exists
(file/dir/symlink) remove it entirely and then create a fresh real directory;
when creating per-repo entries in that directory (the loop under if name ==
"openwisp-docs"), detect if the repo's docs lives under a docs/ subdirectory
(e.g., conf.py or docs/conf.py exists) and make the symlink point to that docs
directory rather than the repository root so behavior matches
clone_or_update_repo() and avoids polluting worktrees.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.py
🧰 Additional context used
🪛 Ruff (0.14.13)
build.py
315-315: Starting a process with a partial executable path
(S607)
321-321: subprocess call: check for execution of untrusted input
(S603)
322-328: Starting a process with a partial executable path
(S607)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (1)
build.py (1)
315-328: Git fetch/checkout simplification looks good.The simplified fetch and detached-head suppression should handle both branches and tags cleanly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I have a couple of questions.
| if os.path.exists(clone_path): | ||
| print(f"Repository '{name}' already exists. Updating...") | ||
| subprocess.run( | ||
| ["git", "fetch", "origin", f"refs/heads/{branch}:refs/heads/{branch}"], |
There was a problem hiding this comment.
This change was done to deal with the case when branches and tags have the same name, why are you changing this?
There was a problem hiding this comment.
i hit a 'refusing to fetch into checked out branch' error during testing and edited the line this way to fix it
this code only runs for other repos though and the symlink approach already handles openwisp-docs itself so ill double check this
| """ | ||
| repository = f"{owner}/{name}" | ||
| # Support for building with local changes | ||
| if name == "openwisp-docs" and not os.environ.get("PRODUCTION"): |
There was a problem hiding this comment.
Why when PRODUCTION is set the behavior is different?
There was a problem hiding this comment.
i added the PRODUCTION check thinking CI might need to clone from github but i just lookedat the CI workflow and it already checks out the specific branch before building so using local files would work fine in CI too
ill remove this check
| # "-c advice.detachedHead=false" is used to suppress the warning | ||
| # about being in a detached HEAD state when checking out tags. | ||
| subprocess.run( | ||
| ["git", "-c", "advice.detachedHead=false", "checkout", f"refs/heads/{branch}"], |
nemesifier
changed the title
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Please rebase on the latest master (due to 972e9e1).
If you run into issues while running that command locally, I am open to adding exception handling to resolve it.
|
on it right now,
|
Contributors could not preview local documentation changes because the build system always cloned from GitHub instead of using local files. This fix modifies build.py to detect local openwisp-docs builds and symlink local files to staging-dir/ instead of cloning. This works only in non-PRODUCTION mode, so CI/CD behavior is unchanged. Also fixed git fetch/checkout commands to work with branches and tags. Fixes openwisp#251
atif09
force-pushed
the
issues/251-build-with-local-changes
branch
from
601b757 to
346020a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build.py (1)
315-333: Fetch/checkout inconsistency for tag support.The checkout was changed to use
branchdirectly (supporting both branches and tags), but the fetch command at line 317 still usesrefs/heads/{branch}, which only works for branches. Ifbranchis a tag name, the fetch will fail with "couldn't find remote ref". While the config currently uses only branch names, the code comments explicitly acknowledge tag support (e.g., "when checking out tags"), making this an incomplete implementation.Proposed fix
subprocess.run( - # Clears ambiguity when git tags and branches have identical names - ["git", "fetch", "origin", f"refs/heads/{branch}:refs/heads/{branch}"], + ["git", "fetch", "origin", branch], cwd=clone_path, check=True, )
🧹 Nitpick comments (1)
build.py (1)
274-299: Local build pathway looks good overall, with one edge-case gap.The symlink handling for
staging-dirbeing a symlink (lines 278-279) was added per previous feedback. However, there's still no handling ifstaging-direxists as a regular file (not directory or symlink). Additionally, ifdest_pathexists as a regular file or directory (not a symlink), the code silently skips symlink creation without warning.Proposed robustness improvement
- if os.path.islink("staging-dir"): + if os.path.islink("staging-dir") or os.path.isfile("staging-dir"): os.unlink("staging-dir") os.makedirs("staging-dir", exist_ok=True)And for the inner loop, consider warning if a non-symlink file/directory blocks creation:
if os.path.islink(dest_path): os.unlink(dest_path) - if not os.path.exists(dest_path): - os.symlink(src_path, dest_path) + if os.path.exists(dest_path): + print(f"Warning: '{dest_path}' exists and is not a symlink, skipping") + else: + os.symlink(src_path, dest_path)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.py
🧰 Additional context used
🪛 Ruff (0.14.13)
build.py
324-330: Starting a process with a partial executable path
(S607)
⏰ 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). (1)
- GitHub Check: build
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
i rebased to 972e9e1, the git fetch on repeated builds still hits the "refusing to fetch into checked out branch" error when updating other repos, the first build succeeds fine but subsequent builds fail when trying to update existing repos in modules/ |
As suggested by CodeRabbit review, add check to ensure staging-dir is a real directory before creating symlinks inside it.
atif09
force-pushed
the
issues/251-build-with-local-changes
branch
from
346020a to
d98c821
Compare
|
did not add the inner loop warning suggested by code rabbit in the nitpick comments to keep changes minimal
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build.py (1)
314-333: Avoid fetching into a checked-out branch (breaks repeated builds).
git fetch origin refs/heads/{branch}:refs/heads/{branch}will fail on repeated builds because the branch remains the target of refs/heads/{branch} after the initial clone. Git refuses to fetch into a locally checked-out branch to prevent desynchronizing the working tree and index from HEAD. After initial clone checks out the branch, subsequent update cycles fail when attempting to fetch into the same checked-out branch.The proposed fix correctly addresses this by fetching into the remote-tracking ref
refs/remotes/origin/{branch}(which can always be updated) and then resetting the local branch from it usinggit checkout -B. This also improves the workflow by keeping the local branch properly checked out rather than leaving it in a detached state.✅ Proposed fix
- subprocess.run( - # Clears ambiguity when git tags and branches have identical names - ["git", "fetch", "origin", f"refs/heads/{branch}:refs/heads/{branch}"], - cwd=clone_path, - check=True, - ) + subprocess.run( + # Update remote-tracking ref (avoid fetching into checked-out branch) + [ + "git", + "fetch", + "origin", + f"+refs/heads/{branch}:refs/remotes/origin/{branch}", + ], + cwd=clone_path, + check=True, + ) @@ - subprocess.run( - [ - "git", - "-c", - "advice.detachedHead=false", - "checkout", - f"refs/heads/{branch}", - ], - cwd=clone_path, - check=True, - ) + subprocess.run( + [ + "git", + "-c", + "advice.detachedHead=false", + "checkout", + "-B", + branch, + f"refs/remotes/origin/{branch}", + ], + cwd=clone_path, + check=True, + )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.py
🧰 Additional context used
🪛 Ruff (0.14.13)
build.py
324-330: Starting a process with a partial executable path
(S607)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (1)
build.py (1)
274-299: Local symlink staging flow looks solid for dev builds.Clear, minimal logic for staging, exclusions, and pyvenv detection.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
The original refspec tried to fetch directly into checked-out branches, causing 'refusing to fetch into checked-out branch' errors on repeated builds This fix fetches into remote-tracking branches (refs/remotes/origin/) instead, which can always be updated, then uses 'checkout -B' to create/reset the local branch from the remote. This maintains branch/tag disambiguation while fixing the repeated build issue Fixes openwisp#251
|
commit 4b9525a has solved the git fetch error mentioned earlier and the builds are successful both locally and in CI,
|
2 participants
contributors could not preview local documentation changes because the build system always cloned from GitHub instead of using local files
this fix modifies build.py to detect local openwisp-docs builds and symlink local files to staging-dir/ instead of cloning
this works only in non-PRODUCTION mode, so CI/CD behavior is unchanged
also fixed git fetch/checkout commands to work with branches and tags.
Fixes #251
Testing
Test 1: Local changes are included in build
RESULT
Test 2 : SStaging-dir uses symlinks (not cloning)
ls -la staging-dir/ | head -10RESULT
all files are symlinks to local directory
Test 3: Testing production mode
RESULT
production mode still clones to modules/ directory, CI/CD behavior unchanged.