🐛 fix warnings for duplicate needs in parallel builds (#1223)

Change duplicate need feedback from raising exceptions to emitting Sphinx warnings, e.g.

```
path/to/page.rst:4: WARNING: A need with ID STORY_PAGE_1 already exists, title: 'duplicate'. [needs.duplicate_id]
```

and ensure warning is also emitted during parallel builds

Note, although there was already a parallel build test in the test suite, 
actually it was not running parallel because there were not enough documents in the test project
(see sphinx-doc/sphinx#12796), so one more document is added.

Author: chrisjsewell

sphinx_needs/api/exceptions.py

@@ -19,10 +19,6 @@ class NeedsNoIdException(SphinxError):
     pass
 
 
-class NeedsDuplicatedId(SphinxError):
-    pass
-
-
 class NeedsStatusNotAllowed(SphinxError):
     pass
 

sphinx_needs/api/need.py

@@ -16,7 +16,6 @@
 from sphinx_needs.api.configuration import NEEDS_CONFIG
 from sphinx_needs.api.exceptions import (
     NeedsConstraintNotAllowed,
-    NeedsDuplicatedId,
     NeedsInvalidException,
     NeedsInvalidOption,
     NeedsNoIdException,
@@ -312,19 +311,24 @@ def run():
 
     if need_id in SphinxNeedsData(env).get_or_create_needs():
         if id:
-            raise NeedsDuplicatedId(
-                f"A need with ID {need_id} already exists! "
-                f"This is not allowed. Document {docname}[{lineno}] Title: {title}."
-            )
+            message = f"A need with ID {need_id} already exists, " f"title: {title!r}."
         else:  # this is a generated ID
-            raise NeedsDuplicatedId(
+            _title = " ".join(title)
+            message = (
                 "Needs could not generate a unique ID for a need with "
-                "the title '{}' because another need had the same title. "
+                f"the title {_title!r} because another need had the same title. "
                 "Either supply IDs for the requirements or ensure the "
                 "titles are different.  NOTE: If title is being generated "
                 "from the content, then ensure the first sentence of the "
-                "requirements are different.".format(" ".join(title))
+                "requirements are different."
             )
+        logger.warning(
+            message + " [needs.duplicate_id]",
+            type="needs",
+            subtype="duplicate_id",
+            location=(docname, lineno) if docname else None,
+        )
+        return []
 
     # Trim title if it is too long
     max_length = needs_config.max_title_length

sphinx_needs/data.py

@@ -6,6 +6,8 @@
 
 from typing import TYPE_CHECKING, Literal, TypedDict
 
+from sphinx.util.logging import getLogger
+
 if TYPE_CHECKING:
     from docutils.nodes import Element, Text
     from sphinx.application import Sphinx
@@ -15,6 +17,9 @@
     from sphinx_needs.services.manager import ServiceManager
 
 
+LOGGER = getLogger(__name__)
+
+
 class NeedsFilterType(TypedDict):
     filter: str
     """Filter string."""
@@ -612,7 +617,7 @@ def get_or_create_umls(self) -> dict[str, NeedsUmlType]:
 
 
 def merge_data(
-    _app: Sphinx, env: BuildEnvironment, _docnames: list[str], other: BuildEnvironment
+    _app: Sphinx, env: BuildEnvironment, docnames: list[str], other: BuildEnvironment
 ) -> None:
     """
     Performs data merge of parallel executed workers.
@@ -621,13 +626,32 @@ def merge_data(
     Needs to update env manually for all data Sphinx-Needs collect during read phase
     """
 
-    # Update global needs dict
-    needs = SphinxNeedsData(env).get_or_create_needs()
-    other_needs = SphinxNeedsData(other).get_or_create_needs()
-    needs.update(other_needs)
     if SphinxNeedsData(other).has_export_filters:
         SphinxNeedsData(env).has_export_filters = True
 
+    # Update needs
+    needs = SphinxNeedsData(env).get_or_create_needs()
+    other_needs = SphinxNeedsData(other).get_or_create_needs()
+    for other_id, other_need in other_needs.items():
+        if other_id in needs:
+            # we only want to warn if the need comes from one of the docs parsed in this worker
+            _docname = other_need["docname"]
+            if _docname in docnames:
+                message = (
+                    f"A need with ID {other_id} already exists, "
+                    f"title: {other_need['title']!r}."
+                )
+                LOGGER.warning(
+                    message + " [needs.duplicate_id]",
+                    type="needs",
+                    subtype="duplicate_id",
+                    location=(_docname, other_need["lineno"]) if _docname else None,
+                )
+        else:
+            needs[other_id] = other_need
+
+    # update other data
+
     def _merge(name: str, is_complex_dict: bool = False) -> None:
         # Update global needs dict
         if not hasattr(env, name):

sphinx_needs/external_needs.py

@@ -11,7 +11,6 @@
 from sphinx.environment import BuildEnvironment
 
 from sphinx_needs.api import add_external_need, del_need
-from sphinx_needs.api.exceptions import NeedsDuplicatedId
 from sphinx_needs.config import NeedsSphinxConfig
 from sphinx_needs.data import SphinxNeedsData
 from sphinx_needs.logging import get_logger
@@ -30,7 +29,7 @@ def get_target_template(target_url: str) -> Template:
     return mem_template
 
 
-def load_external_needs(app: Sphinx, env: BuildEnvironment, _docname: str) -> None:
+def load_external_needs(app: Sphinx, env: BuildEnvironment, docname: str) -> None:
     """Load needs from configured external sources."""
     needs_config = NeedsSphinxConfig(app.config)
     for source in needs_config.external_needs:
@@ -159,10 +158,14 @@ def load_external_needs(app: Sphinx, env: BuildEnvironment, _docname: str) -> No
                     # delete the already existing external need from api need
                     del_need(app, ext_need_id)
                 else:
-                    raise NeedsDuplicatedId(
-                        f'During external needs handling, an identical ID was detected: {ext_need_id} \
-                            from needs_external_needs url: {source["base_url"]}'
+                    log.warning(
+                        f'During external needs handling, an identical ID was detected: {ext_need_id} '
+                        f'from needs_external_needs url: {source["base_url"]} [needs.duplicate_id]',
+                        type="needs",
+                        subtype="duplicate_id",
+                        location=docname if docname else None,
                     )
+                    return None
 
             add_external_need(app, **need_params)
 

tests/doc_test/parallel_doc/index.rst

@@ -7,6 +7,7 @@ PARALLEL TEST DOCUMENT
    page_2
    page_3
    page_4
+   page_5
 
 .. spec:: Command line interface
     :id: SP_TOO_001

tests/doc_test/parallel_doc/page_5.rst

@@ -0,0 +1,5 @@
+Page 5
+======
+
+.. story:: duplicate
+   :id: STORY_PAGE_1

tests/test_broken_doc.py

@@ -1,18 +1,26 @@
-import pytest
+import os
 
-from sphinx_needs.api.need import NeedsDuplicatedId
+import pytest
+from sphinx.testing.util import SphinxTestApp
+from sphinx.util.console import strip_colors
 
 
 @pytest.mark.parametrize(
     "test_app",
-    [{"buildername": "html", "srcdir": "doc_test/broken_doc"}],
+    [{"buildername": "html", "srcdir": "doc_test/broken_doc", "no_plantuml": True}],
     indirect=True,
 )
-def test_doc_build_html(test_app):
-    with pytest.raises(NeedsDuplicatedId):
-        app = test_app
-
-        app.build()
-        html = (app.outdir / "index.html").read_text()
-        assert "<h1>BROKEN DOCUMENT" in html
-        assert "SP_TOO_001" in html
+def test_doc_build_html(test_app: SphinxTestApp):
+    test_app.build()
+    warnings = (
+        strip_colors(test_app._warning.getvalue())
+        .replace(str(test_app.srcdir) + os.path.sep, "<srcdir>/")
+        .strip()
+    )
+    assert (
+        warnings
+        == "<srcdir>/index.rst:11: WARNING: A need with ID SP_TOO_001 already exists, title: 'Command line interface'. [needs.duplicate_id]"
+    )
+    html = (test_app.outdir / "index.html").read_text()
+    assert "<h1>BROKEN DOCUMENT" in html
+    assert "SP_TOO_001" in html

tests/test_parallel_execution.py

@@ -1,31 +1,38 @@
+import os
 from pathlib import Path
 
 import pytest
+from sphinx.util.console import strip_colors
 
 
 @pytest.mark.parametrize(
     "test_app",
-    [{"buildername": "html", "srcdir": "doc_test/parallel_doc", "parallel": 4}],
+    [
+        {
+            "buildername": "html",
+            "srcdir": "doc_test/parallel_doc",
+            "parallel": 4,
+            "no_plantuml": True,
+        }
+    ],
     indirect=True,
 )
 def test_doc_build_html(test_app):
     app = test_app
     app.build()
-    html = Path(app.outdir, "index.html").read_text()
-    assert app.statuscode == 0
-    assert "<h1>PARALLEL TEST DOCUMENT" in html
-    assert "SP_TOO_001" in html
-
+    warnings = (
+        strip_colors(app._warning.getvalue())
+        .replace(str(app.srcdir) + os.path.sep, "<srcdir>/")
+        .strip()
+    )
+    assert (
+        warnings
+        == "<srcdir>/page_5.rst:4: WARNING: A need with ID STORY_PAGE_1 already exists, title: 'duplicate'. [needs.duplicate_id]"
+    )
 
-@pytest.mark.parametrize(
-    "test_app",
-    [{"buildername": "html", "srcdir": "doc_test/parallel_doc", "parallel": 4}],
-    indirect=True,
-)
-def test_doc_parallel_build_html(test_app):
-    app = test_app
-    app.build()
-    assert app.statuscode == 0
+    index_html = Path(app.outdir, "index.html").read_text()
+    assert "<h1>PARALLEL TEST DOCUMENT" in index_html
+    assert "SP_TOO_001" in index_html
 
     page3_html = Path(app.outdir, "page_3.html").read_text()
     assert "Page 3" in page3_html