[fix] Allow easily building docs with local changes again #251 #257
Conversation
📝 WalkthroughWalkthroughAdds version-aware local-development handling to clone_or_update_repo in build.py: when PRODUCTION is unset and version_name indicates a dev version, the script ensures staging-dir is a real empty directory (removing any existing file/symlink), creates symlinks from base_dir to staging-dir for top-level items excluding staging-dir, modules, _build, .git, pycache, and virtual environments, and returns early to use that staging-dir instead of cloning. For existing openwisp-docs clones, fetch updates the remote-tracking ref for the branch and checkout uses 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, version_name=dev)
Build->>FS: remove existing staging-dir if present and create empty staging-dir
Build->>Dev: determine base_dir (docs if present, else repo root)
loop for each top-level item in base_dir
alt item allowed (not excluded, not hidden, not venv)
Build->>FS: create symlink -> base_dir/item
else
Build-->>FS: skip item
end
end
Build->>Build: return early and use staging-dir for build (bypass clone/update)
sequenceDiagram
participant Build as build.py
participant Git as Remote Git
participant Repo as Local clone dir
Build->>Git: git fetch origin <branch> (update remote-tracking ref)
Git-->>Build: FETCH_HEAD updated
Build->>Repo: git -c advice.detachedHead=false -B <branch> FETCH_HEAD
Repo-->>Build: branch reset to fetched ref
alt on-branch and not production
Build->>Git: git pull origin <branch>
Git-->>Build: branch merged with origin/<branch>
end
Build->>Build: continue build using checked-out branch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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.
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,
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@atif09 see my comments below.
Removed redundant print statement and blank lines Fixes openwisp#251
Fully reset staging-dir and honor docs/ convention Fixes openwisp#251
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@build.py`:
- Around line 319-340: The fetch/checkout logic in build.py currently hardcodes
a refs/heads refspec and fails for tag-based refs; change the git fetch call
that uses f"+refs/heads/{branch}:refs/remotes/origin/{branch}" to a simple "git
fetch origin {branch}" (i.e. subprocess.run(["git","fetch","origin", branch],
cwd=clone_path, check=True)) so Git can resolve branch or tag, and update the
subsequent checkout subprocess.run call that currently checks out
f"refs/remotes/origin/{branch}" to instead checkout the fetched commit (use
"-B", branch, "FETCH_HEAD") so tags and branches both work; locate these changes
around the subprocess.run calls that reference branch and clone_path in
build.py.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.py
🧰 Additional context used
🪛 Ruff (0.14.14)
build.py
320-325: Starting a process with a partial executable path
(S607)
331-331: subprocess call: check for execution of untrusted input
(S603)
332-340: 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)
11-11: LGTM:shutilimport is justified by staging-dir cleanup.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Fix get fetch/checkout to support both branches and tags - Fix git pull to explicitly specify remote and branch Fixes openwisp#251
The PRODUCTION check and branch filter ensure: - Local dev/master builds: use symlinks for testing local changes - Local old version builds (22.05, etc.): clone from GitHub - CI builds (PRODUCTION=1): clone from GitHub for all versions Fixes openwisp#251
…s related to formatting openwisp#251 - Removed PRODUCTION check as advised - Improved format Fixes openwisp#251
|
the QA checks are passing locally but the CI keeps failing due to the same error related to ansible again and again |
|
the CI failure i was coming across was this: so i added the version_name parameter and this makes the version 22.05 clone from v22.05 branch and v22.05 doesn't reference ansible and since this makes the docs and config match, the build finishes without any errors |
Restrict local file symlinks to dev version only, historical versions now clone from their respective branches to ensure correct module structure Fixes openwisp#251
atif09
force-pushed
the
issues/251-build-with-local-changes
branch
from
b9bb3ce to
3bea3c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@build.py`:
- Around line 287-307: The current loop that builds the dev staging directory
(variables base_dir, exclude_items) skips any non-.rst files and directories
starting with "_" which drops conf.py and Sphinx asset dirs like _static and
_templates; update the conditional so it does not continue for conf.py and known
Sphinx asset dirs: explicitly allow item == "conf.py" and allow directories
named "_static" and "_templates" (and keep "spelling_wordlist.txt" allowed), and
keep excluding the original exclude_items; modify the any(...) predicate in the
for loop to check for these exceptions before skipping items so non-RST assets
and conf.py are copied into the staging-dir.
- Around line 277-286: The current logic creates/removes the "staging-dir"
whenever name == "openwisp-docs" and version_name == "dev" even in production;
update both conditionals that touch "staging-dir" to also check that the build
is not a production build (e.g., os.environ.get("PRODUCTION") != "1" or a
boolean PRODUCTION flag is False). Specifically, change the first block that
unlinks/rmtree "staging-dir" (currently guarded by name == "openwisp-docs") to
require name == "openwisp-docs" and not production, and change the second block
that os.makedirs("staging-dir", exist_ok=True) (currently guarded by name ==
"openwisp-docs" and version_name == "dev") to require name == "openwisp-docs",
version_name == "dev", and not production so production builds cannot use the
working tree.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build.py
🧰 Additional context used
🪛 Ruff (0.14.14)
build.py
[error] 336-341: Starting a process with a partial executable path
(S607)
[error] 347-347: subprocess call: check for execution of untrusted input
(S603)
[error] 348-356: Starting a process with a partial executable path
(S607)
[error] 366-366: subprocess call: check for execution of untrusted input
(S603)
[error] 367-367: 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 (3)
build.py (3)
11-11: No concerns with the new import.
334-368: Fetch/checkout update flow looks solid.
454-459: Passingversion_namethrough is correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Support for building with local changes | ||
| if name == "openwisp-docs": | ||
| # Ensure staging-dir is a real, empty directory | ||
| if os.path.islink("staging-dir") or os.path.isfile("staging-dir"): | ||
| os.unlink("staging-dir") | ||
| elif os.path.isdir("staging-dir"): | ||
| shutil.rmtree("staging-dir") | ||
| # If dev version, copy all doc files to the staging dir | ||
| if name == "openwisp-docs" and version_name == "dev": | ||
| os.makedirs("staging-dir", exist_ok=True) |
There was a problem hiding this comment.
Gate the local-dev staging path to non‑PRODUCTION builds.
Right now the dev symlink path triggers whenever version_name == "dev", even if PRODUCTION=1, which violates the stated constraint and can make production builds use the working tree instead of a clean clone.
Proposed fix
- if name == "openwisp-docs" and version_name == "dev":
+ if (
+ name == "openwisp-docs"
+ and version_name == "dev"
+ and not os.environ.get("PRODUCTION")
+ ):🤖 Prompt for AI Agents
In `@build.py` around lines 277 - 286, The current logic creates/removes the
"staging-dir" whenever name == "openwisp-docs" and version_name == "dev" even in
production; update both conditionals that touch "staging-dir" to also check that
the build is not a production build (e.g., os.environ.get("PRODUCTION") != "1"
or a boolean PRODUCTION flag is False). Specifically, change the first block
that unlinks/rmtree "staging-dir" (currently guarded by name == "openwisp-docs")
to require name == "openwisp-docs" and not production, and change the second
block that os.makedirs("staging-dir", exist_ok=True) (currently guarded by name
== "openwisp-docs" and version_name == "dev") to require name ==
"openwisp-docs", version_name == "dev", and not production so production builds
cannot use the working tree.
There was a problem hiding this comment.
@coderabbitai PRODUCTION=1 means this is running the github actions CI, which is always a fresh clone. Moreover, we want the CI build to run this code so that if it's broken the CI build will fail and the problem won't be hidden.
There was a problem hiding this comment.
@nemesifier, thank you for the clarification! You're absolutely right—in CI with PRODUCTION=1, the repository is a fresh clone, so the staging-dir symlinks would point to the same committed files that a normal clone would check out. There are no uncommitted local changes in that context, and running this code path in CI validates that the feature doesn't break. My concern doesn't apply here. I appreciate the explanation!
🐰✨
| base_dir = "docs" if os.path.isdir("docs") else "." | ||
| exclude_items = ("staging-dir", "modules", "version_switcher") | ||
| for item in os.listdir(base_dir): | ||
| is_dir = os.path.isdir(os.path.join(base_dir, item)) | ||
| if any( | ||
| ( | ||
| # not a dir nor a rst file, nor the spelling wordlist | ||
| all( | ||
| ( | ||
| not is_dir, | ||
| not item.endswith(".rst"), | ||
| item != "spelling_wordlist.txt", | ||
| ) | ||
| ), | ||
| # it's a dir but not designed to contain rst files | ||
| is_dir and item.startswith((".", "_")), | ||
| # in the exclude list | ||
| item in exclude_items, | ||
| ) | ||
| ): | ||
| continue |
There was a problem hiding this comment.
Include conf.py and non‑RST assets in dev staging-dir.
The current filter only allows .rst files and excludes underscore dirs, which drops conf.py and folders like _static/_templates; Sphinx will fail or miss assets.
Proposed fix
- exclude_items = ("staging-dir", "modules", "version_switcher")
+ exclude_items = ("staging-dir", "modules", "version_switcher", "_build", "__pycache__")
+ allowed_underscore_dirs = {"_static", "_templates"}
for item in os.listdir(base_dir):
is_dir = os.path.isdir(os.path.join(base_dir, item))
- if any(
- (
- # not a dir nor a rst file, nor the spelling wordlist
- all(
- (
- not is_dir,
- not item.endswith(".rst"),
- item != "spelling_wordlist.txt",
- )
- ),
- # it's a dir but not designed to contain rst files
- is_dir and item.startswith((".", "_")),
- # in the exclude list
- item in exclude_items,
- )
- ):
- continue
+ if item in exclude_items:
+ continue
+ if is_dir:
+ if item.startswith("."):
+ continue
+ if item.startswith("_") and item not in allowed_underscore_dirs:
+ continue
+ elif item.startswith("."):
+ continue🤖 Prompt for AI Agents
In `@build.py` around lines 287 - 307, The current loop that builds the dev
staging directory (variables base_dir, exclude_items) skips any non-.rst files
and directories starting with "_" which drops conf.py and Sphinx asset dirs like
_static and _templates; update the conditional so it does not continue for
conf.py and known Sphinx asset dirs: explicitly allow item == "conf.py" and
allow directories named "_static" and "_templates" (and keep
"spelling_wordlist.txt" allowed), and keep excluding the original exclude_items;
modify the any(...) predicate in the for loop to check for these exceptions
before skipping items so non-RST assets and conf.py are copied into the
staging-dir.
There was a problem hiding this comment.
@coderabbitai I'm testing locally and it's working perfectly. Are you sure sphinx doesn't already look also in the root dir for config files and templates?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find conf.py location(s) in the repository
echo "=== Finding conf.py ==="
fd -t f '^conf\.py$'
echo -e "\n=== Finding _static directory ==="
fd -t d '^_static$' --max-depth 3
echo -e "\n=== Finding _templates directory ==="
fd -t d '^_templates$' --max-depth 3
echo -e "\n=== Repository structure at root ==="
ls -la | head -20
echo -e "\n=== Check if docs directory exists ==="
if [ -d "docs" ]; then
echo "docs directory exists"
echo "Contents of docs:"
ls -la docs/ | head -20
else
echo "docs directory does not exist"
fi
Length of output: 1384
🌐 Web query:
Sphinx conf.py search locations parent directory
💡 Result:
In Sphinx, conf.py is executed with the current working directory set to the directory containing conf.py (your docs “source” dir). That means relative paths like '..' are interpreted relative to conf.py’s folder. (sphinx-themed.readthedocs.io)
To make Sphinx/autodoc “search” (import) modules from a parent directory, prepend that directory to Python’s import path (sys.path) in conf.py:
# docs/source/conf.py
import sys
from pathlib import Path
# add repo root (parent of docs/source) to import path
sys.path.insert(0, str(Path(__file__).resolve().parents[2]))Common “src layout” example (repo has docs/source/conf.py and code in src/your_pkg):
import sys
from pathlib import Path
sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "src"))This is the standard Sphinx-recommended approach when modules aren’t otherwise importable (e.g., not installed in the build env). (sphinx-doc.org)
Citations:
- 1: https://sphinx-themed.readthedocs.io/en/latest/usage/configuration.html?utm_source=openai
- 2: https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Check the Makefile to see how Sphinx is invoked
echo "=== Makefile content (first 100 lines) ==="
head -100 Makefile
echo -e "\n=== Search for SRCDIR and sphinx-build in Makefile ==="
rg -n "sphinx-build|SRCDIR" Makefile | head -20
Length of output: 4454
@nemesifier You're absolutely right, and I apologize for the confusion.
Looking at the Makefile, I can see that Sphinx is invoked with -c . which tells it to look for conf.py in the root directory, while SRCDIR (passed as staging-dir by build.py) specifies where to find the RST documentation sources:
ALLSPHINXOPTS = -c . -d $(BUILDDIR)/doctrees ... $(SRCDIR)So the current approach is correct:
conf.py,_static/, and_templates/remain in the repository root- Only the RST files and
spelling_wordlist.txtare symlinked intostaging-dir/ - Sphinx finds the config and assets in root via
-c ., and the documentation sources instaging-dir
The filtering logic is working exactly as intended. My original review comment was mistaken—there's no need to include conf.py or the underscore directories in the staging directory.
Thank you for testing locally and pushing back on this! 🙏
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
2 participants
Contributors could not preview local documentation changes because the build system always cloned from GitHub
This fix ensures that build.py uses local files when building the dev version which allows contributors to verify changes before submitting PRs
Fixes #251
Testing
Result