Skip to content

[fix] Allow easily building docs with local changes again #251 #257

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.

Sign up for GitHub

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

Merged
nemesifier merged 12 commits into from
Jan 30, 2026
Merged

[fix] Allow easily building docs with local changes again #251 #257

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ab4b835
[build] Enable building docs with local changes #251
atif09 Jan 17, 2026
d98c821
[build] Add safeguard for staging-dir symlink edge case
atif09 Jan 17, 2026
4b9525a
[build] Fix git fetch for repeated builds #251
atif09 Jan 18, 2026
2e8ae91
Merge branch 'master' into issues/251-build-with-local-changes
atif09 Jan 29, 2026
1749f96
[fix] Addressing feedback #251
atif09 Jan 29, 2026
bc1aa8f
[fix] Addressing missed code rabbit feedback #251
atif09 Jan 29, 2026
662b074
[fix] Addressing missed code rabbit feedback #251
atif09 Jan 29, 2026
09a9879
[fix] Add PRODUCTION check and branch filter (master only) #251
atif09 Jan 29, 2026
8a41477
[fix] Ran build locally without PRODUCTION check and other minor fixe…
atif09 Jan 29, 2026
3bea3c2
[fix] Use local files only for dev version builds #251
atif09 Jan 30, 2026
c8ec441
Merge branch 'master' into issues/251-build-with-local-changes
nemesifier Jan 30, 2026
885390d
[chores] Minor improvemnts
nemesifier Jan 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 67 additions & 5 deletions build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import argparse
import os
import shutil
import subprocess
from copy import deepcopy

Expand Down Expand Up @@ -265,12 +266,57 @@ def git_is_on_branch(repo_path):
return result.stdout.strip() != "HEAD"


def clone_or_update_repo(name, branch, dir_name, owner="openwisp", dest=None):
def clone_or_update_repo(
name, branch, dir_name, owner="openwisp", dest=None, version_name=None
):
"""
Clone or update a repository based on the module name and branch provided.
If the repository already exists, update it. Otherwise, clone the repository.
"""
repository = f"{owner}/{name}"
# 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)
Comment on lines +277 to +286

@coderabbitai coderabbitai Bot Jan 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

@nemesifier nemesifier Jan 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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.

@coderabbitai coderabbitai Bot Jan 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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
Comment on lines +287 to +307

@coderabbitai coderabbitai Bot Jan 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

@nemesifier nemesifier Jan 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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?

@coderabbitai coderabbitai Bot Jan 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 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:

🏁 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.txt are symlinked into staging-dir/
  • Sphinx finds the config and assets in root via -c ., and the documentation sources in staging-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! 🙏

# skip virtual environments (detect with pyvenv.cfg)
if os.path.isdir(os.path.join(base_dir, item)) and os.path.exists(
os.path.join(base_dir, item, "pyvenv.cfg")
):
continue
src_path = os.path.abspath(os.path.join(base_dir, item))
dest_path = os.path.join("staging-dir", item)
if os.path.islink(dest_path):
os.unlink(dest_path)
if not os.path.exists(dest_path):
os.symlink(src_path, dest_path)
return
if os.environ.get("SSH"):
# SSH cloning is a convenient option for local development, as it
# allows you to commit changes directly to the repository, but it
Expand All @@ -286,15 +332,28 @@ def clone_or_update_repo(name, branch, dir_name, owner="openwisp", dest=None):
if os.path.exists(clone_path):
print(f"Repository '{name}' already exists. Updating...")
subprocess.run(
# Clears ambiguity when git tags and branches have identical names
["git", "fetch", "origin", f"refs/heads/{branch}:refs/heads/{branch}"],
# Update remote-tracking ref (avoid fetching into checked-out branch)
[
"git",
"fetch",
"origin",
branch,
],
cwd=clone_path,
check=True,
)
# "-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}"],
[
"git",
"-c",
"advice.detachedHead=false",
"checkout",
"-B",
branch,
"FETCH_HEAD",
],
Comment thread
coderabbitai[bot] marked this conversation as resolved.
cwd=clone_path,
check=True,
)
Expand All @@ -304,7 +363,9 @@ def clone_or_update_repo(name, branch, dir_name, owner="openwisp", dest=None):
# During local development, we attempt to pull updates, but only if the
# current HEAD is on a branch (i.e., not detached, such as when on a tag).
if not os.environ.get("PRODUCTION", False) and git_is_on_branch(clone_path):
subprocess.run(["git", "pull"], cwd=clone_path, check=True)
subprocess.run(
["git", "pull", "origin", branch], cwd=clone_path, check=True
)
else:
print(f"Cloning repository '{name}'...")
subprocess.run(
Expand Down Expand Up @@ -394,6 +455,7 @@ def main():
name="openwisp-docs",
branch=docs_branch,
dir_name="openwisp-docs",
version_name=version_name,
)
for module in modules:
clone_or_update_repo(
Expand Down