Merge pull request #1098 from carmenbianca/fix-plus

Fix downloading `SPDX-IDENTIFIER+`

Author: carmenbianca

changelog.d/fixed/strip-plus.md

@@ -0,0 +1,2 @@
+- When running `reuse download SPDX-IDENTIFIER+`, download `SPDX-IDENTIFIER`
+  instead. This also works for `reuse download --all`. (#1098)

src/reuse/_util.py

@@ -109,6 +109,32 @@ def _determine_license_suffix_path(path: StrPath) -> Path:
     return Path(f"{path}.license")
 
 
+def _strip_plus_from_identifier(spdx_identifier: str) -> str:
+    """Strip final plus from identifier.
+
+    >>> _strip_plus_from_identifier("EUPL-1.2+")
+    'EUPL-1.2'
+    >>> _strip_plus_from_identifier("EUPL-1.2")
+    'EUPL-1.2'
+    """
+    if spdx_identifier.endswith("+"):
+        return spdx_identifier[:-1]
+    return spdx_identifier
+
+
+def _add_plus_to_identifier(spdx_identifier: str) -> str:
+    """Add final plus to identifier.
+
+    >>> _add_plus_to_identifier("EUPL-1.2")
+    'EUPL-1.2+'
+    >>> _add_plus_to_identifier("EUPL-1.2+")
+    'EUPL-1.2+'
+    """
+    if spdx_identifier.endswith("+"):
+        return spdx_identifier
+    return f"{spdx_identifier}+"
+
+
 def relative_from_root(path: StrPath, root: StrPath) -> Path:
     """A helper function to get *path* relative to *root*."""
     path = Path(path)

src/reuse/cli/download.py

@@ -15,6 +15,7 @@
 import click
 
 from .._licenses import ALL_NON_DEPRECATED_MAP
+from .._util import _strip_plus_from_identifier
 from ..download import _path_to_license_file, put_license_in_file
 from ..i18n import _
 from ..report import ProjectReport
@@ -138,7 +139,7 @@ def _successfully_downloaded(destination: StrPath) -> None:
     ),
 )
 @click.argument(
-    "license_",
+    "licenses",
     # TRANSLATORS: You may translate this. Please preserve capital letters.
     metavar=_("LICENSE"),
     type=str,
@@ -147,22 +148,20 @@ def _successfully_downloaded(destination: StrPath) -> None:
 @click.pass_obj
 def download(
     obj: ClickObj,
-    license_: Collection[str],
+    licenses: Collection[str],
     all_: bool,
     output: Optional[Path],
     source: Optional[Path],
 ) -> None:
     # pylint: disable=missing-function-docstring
-    if all_ and license_:
+    if all_ and licenses:
         raise click.UsageError(
             _(
                 "The 'LICENSE' argument and '--all' option are mutually"
                 " exclusive."
             )
         )
 
-    licenses: Collection[str] = license_  # type: ignore
-
     if all_:
         # TODO: This is fairly inefficient, but gets the job done.
         report = ProjectReport.generate(obj.project, do_checksum=False)
@@ -173,6 +172,7 @@ def download(
             _("Cannot use '--output' with more than one license.")
         )
 
+    licenses = {_strip_plus_from_identifier(lic) for lic in licenses}
     return_code = 0
     for lic in licenses:
         destination: Path = output  # type: ignore

src/reuse/report.py

@@ -32,7 +32,11 @@
 from uuid import uuid4
 
 from . import _LICENSING, __REUSE_version__, __version__
-from ._util import _checksum
+from ._util import (
+    _add_plus_to_identifier,
+    _checksum,
+    _strip_plus_from_identifier,
+)
 from .extract import _LICENSEREF_PATTERN
 from .global_licensing import ReuseDep5
 from .i18n import _
@@ -436,15 +440,13 @@ def unused_licenses(self) -> set[str]:
         if self._unused_licenses is not None:
             return self._unused_licenses
 
-        # First collect licenses that are suspected to be unused.
-        suspected_unused_licenses = {
-            lic for lic in self.licenses if lic not in self.used_licenses
-        }
-        # Remove false positives.
         self._unused_licenses = {
             lic
-            for lic in suspected_unused_licenses
-            if f"{lic}+" not in self.used_licenses
+            for lic in self.licenses
+            if not any(
+                identifier in self.used_licenses
+                for identifier in set((lic, _add_plus_to_identifier(lic)))
+            )
         }
         return self._unused_licenses
 
@@ -750,8 +752,12 @@ def generate(
                     # A license expression akin to Apache-1.0+ should register
                     # correctly if LICENSES/Apache-1.0.txt exists.
                     identifiers = {identifier}
-                    if identifier.endswith("+"):
-                        identifiers.add(identifier[:-1])
+                    if (
+                        plus_identifier := _strip_plus_from_identifier(
+                            identifier
+                        )
+                    ) != identifier:
+                        identifiers.add(plus_identifier)
                     # Bad license
                     if not identifiers.intersection(project.license_map):
                         report.bad_licenses.add(identifier)

tests/test_cli_download.py

@@ -4,10 +4,11 @@
 
 """Tests for download."""
 
-# pylint: disable=redefined-outer-name,unused-argument
+# pylint: disable=redefined-outer-name,unused-argument,unspecified-encoding
 
 import errno
 import os
+import shutil
 from pathlib import Path
 from unittest.mock import create_autospec
 from urllib.error import URLError
@@ -18,6 +19,8 @@
 from reuse.cli import download
 from reuse.cli.main import main
 
+# REUSE-IgnoreStart
+
 
 @pytest.fixture()
 def mock_put_license_in_file(monkeypatch):
@@ -39,6 +42,67 @@ def test_simple(self, empty_directory, mock_put_license_in_file):
             "0BSD", Path(os.path.realpath("LICENSES/0BSD.txt")), source=None
         )
 
+    def test_strip_plus(self, empty_directory, mock_put_license_in_file):
+        """If downloading LIC+, download LIC instead."""
+        result = CliRunner().invoke(main, ["download", "EUPL-1.2+"])
+
+        assert result.exit_code == 0
+        mock_put_license_in_file.assert_called_with(
+            "EUPL-1.2",
+            Path(os.path.realpath("LICENSES/EUPL-1.2.txt")),
+            source=None,
+        )
+
+    def test_all(self, fake_repository, mock_put_license_in_file):
+        """--all downloads all detected licenses."""
+        shutil.rmtree("LICENSES")
+        result = CliRunner().invoke(main, ["download", "--all"])
+
+        assert result.exit_code == 0
+        for lic in [
+            "GPL-3.0-or-later",
+            "LicenseRef-custom",
+            "Autoconf-exception-3.0",
+            "Apache-2.0",
+            "CC0-1.0",
+        ]:
+            mock_put_license_in_file.assert_any_call(
+                lic, Path(os.path.realpath(f"LICENSES/{lic}.txt")), source=None
+            )
+
+    def test_all_with_plus(self, fake_repository, mock_put_license_in_file):
+        """--all downloads EUPL-1.2 if EUPL-1.2+ is detected."""
+        Path("foo.py").write_text("# SPDX-License-Identifier: EUPL-1.2+")
+        result = CliRunner().invoke(main, ["download", "--all"])
+
+        assert result.exit_code == 0
+        mock_put_license_in_file.assert_called_once_with(
+            "EUPL-1.2",
+            Path(os.path.realpath("LICENSES/EUPL-1.2.txt")),
+            source=None,
+        )
+
+    def test_all_with_plus_and_non_plus(
+        self, fake_repository, mock_put_license_in_file
+    ):
+        """If both EUPL-1.2 and EUPL-1.2+ is detected, download EUPL-1.2 only
+        once.
+        """
+        Path("foo.py").write_text(
+            """
+            # SPDX-License-Identifier: EUPL-1.2+
+            # SPDX-License-Identifier: EUPL-1.2
+            """
+        )
+        result = CliRunner().invoke(main, ["download", "--all"])
+
+        assert result.exit_code == 0
+        mock_put_license_in_file.assert_called_once_with(
+            "EUPL-1.2",
+            Path(os.path.realpath("LICENSES/EUPL-1.2.txt")),
+            source=None,
+        )
+
     def test_all_and_license_mutually_exclusive(self, empty_directory):
         """--all and license args are mutually exclusive."""
         result = CliRunner().invoke(main, ["download", "--all", "0BSD"])
@@ -223,3 +287,6 @@ def test_prefix(self):
         result = download._similar_spdx_identifiers("CC0")
 
         assert "CC0-1.0" in result
+
+
+# REUSE-IgnoreEnd