Address review comments and add misc refactors

Signed-off-by: Ayan Sinha Mahapatra <ayansmahapatra@gmail.com>

Author: AyanSinhaMahapatra

src/licensedcode/cache.py

@@ -515,6 +515,9 @@ def build_spdx_license_expression(license_expression, licensing=None):
     >>> spdx = "MIT OR GPL-2.0-only WITH LicenseRef-scancode-generic-exception"
     >>> assert build_spdx_license_expression(exp) == spdx
     """
+    if not license_expression:
+        return
+
     if not licensing:
         licensing = get_licensing()
     validate_spdx_license_keys(license_expression=license_expression, licensing=licensing)

src/licensedcode/detection.py

@@ -1730,9 +1730,8 @@ def get_ambiguous_license_detections_by_type(unique_license_detections):
         ):
             ambi_license_detections[DetectionCategory.LICENSE_CLUES.value] = detection
 
-        elif "unknown" in detection.license_expression:
-            if has_unknown_matches(license_matches=detection.matches):
-                ambi_license_detections[DetectionCategory.UNKNOWN_MATCH.value] = detection
+        elif detection.is_unknown:
+            ambi_license_detections[DetectionCategory.UNKNOWN_MATCH.value] = detection
 
         elif is_match_coverage_less_than_threshold(
             license_matches=detection.matches,
@@ -1876,14 +1875,15 @@ def get_referenced_filenames(license_matches):
 
 def has_resolved_referenced_file(license_matches):
     """
-    Return a list of unique referenced filenames found in the rules of a list of
-    ``license_matches``
+    Return True if a list of ``license_matches`` has matches from both the original
+    files and the referenced files. This would mean that the license reference is
+    resolved successfully.
     """
     match_origin_files = list(set([
         license_match.from_file
         for license_match in license_matches
     ]))
-    if len(match_origin_files) == 2:
+    if len(match_origin_files) >= 2:
         return True
     else:
         return False
@@ -1894,16 +1894,20 @@ def find_referenced_resource_from_package(referenced_filename, resource, codebas
     Return a Resource matching the ``referenced_filename`` path or filename
     given a ``resource`` in ``codebase``.
 
+    To find the `referenced_filename` the sibling files are searched beside all the
+    package manifest paths, for all the packages which the resource is a part of,
+    to resolve references to files in package ecosystem specific locations.
+
     Return None if the ``referenced_filename`` cannot be found in the same
     directory as the base ``resource``, or at the codebase ``root``.
 
     ``referenced_filename`` is the path or filename referenced in a
     LicenseMatch detected at ``resource``,
     """
-    if not resource:
+    codebase_packages = codebase.attributes.packages
+    if not (resource and codebase_packages):
         return
 
-    codebase_packages = codebase.attributes.packages
     datafile_paths_by_package_uid = {}
     for package in codebase_packages:
         package_uid = package.get("package_uid")
@@ -1940,6 +1944,9 @@ def find_referenced_resource(referenced_filename, resource, codebase, **kwargs):
     Return a Resource matching the ``referenced_filename`` path or filename
     given a ``resource`` in ``codebase``.
 
+    To find the `referenced_filename` the sibling files of the `resource`
+    and files at the `codebase` root are searched.
+
     Return None if the ``referenced_filename`` cannot be found in the same
     directory as the base ``resource``, or at the codebase ``root``.
 
@@ -1969,7 +1976,14 @@ def find_referenced_resource(referenced_filename, resource, codebase, **kwargs):
 
 
 def update_expressions_from_license_detections(resource, codebase):
+    """
+    Set the `detected_license_expression` and `detected_license_expression_spdx`
+    for a `resource` from the individual license_expressions of it's detections.
 
+    This needs to be executed after license detections in a resource are modified,
+    for example when detections are updated with license matches from a resolved
+    reference to a file.
+    """
     license_expressions = [
         detection["license_expression"]
         for detection in resource.license_detections
@@ -1997,8 +2011,18 @@ def update_expressions_from_license_detections(resource, codebase):
     return resource
 
 
-def update_detection_from_referenced_files(referenced_filenames, license_detection_mapping, resource, codebase, analysis, find_referenced_resource_func):
-    
+def update_detection_from_referenced_files(
+    referenced_filenames,
+    license_detection_mapping,
+    resource,
+    codebase,
+    analysis,
+    find_referenced_resource_func,
+):
+    """
+    Return True if the `license_detection_mapping` was updated with resolved
+    license references to other `referenced_filenames`, or return False otherwise.
+    """
     license_detection = LicenseDetectionFromResult.from_license_detection_mapping(
         license_detection_mapping=license_detection_mapping,
         file_path=resource.path,

src/packagedcode/licensing.py

@@ -336,7 +336,11 @@ def add_license_from_sibling_file(resource, codebase):
 
 
 def add_referenced_license_detection_from_package_manifest_siblings(resource, codebase):
-
+    """
+    Return True if we have references to licenses in other files and we could find and resolve
+    the references successfully. Resolving the references mean adding licenses matches from
+    the referenced file and updating the license expressions accordingly.
+    """
     if TRACE:
         logger_debug(f'packagedcode.licensing: add_referenced_license_detection_from_package_manifest_siblings: resource: {resource.path}')
 
@@ -846,8 +850,8 @@ def get_license_detections_and_expression(
     datasource_id=None,
 ):
     """
-    Given a text `extracted_license_statement` return a list of LicenseDetection objects.
-    `extracted_license_statement` is typically found in package manifests.
+    Return a list of LicenseDetection objects from an `extracted_license_statement`
+    text. `extracted_license_statement` is typically found in package manifests.
 
     If `try_as_expression` is True try first to parse this as a license
     expression using the ``expression_symbols`` mapping of {lowered key:
@@ -908,7 +912,7 @@ def get_license_detections_for_extracted_license_statement(
     expression_symbols=None,
 ):
     """
-    Return a list of LicenseDetection detected  the ``extracted_license_statement`` string.
+    Return a list of LicenseDetection detected in the ``extracted_license_statement`` string.
     """
     if not extracted_license_statement:
         return []

src/packagedcode/win_pe.py

@@ -17,6 +17,8 @@
 from packagedcode import models
 from packagedcode.models import Party
 from packagedcode.models import party_org
+from cluecode.copyrights import detect_copyrights_from_lines
+from cluecode.copyrights import prepare_text_line
 from typecode import contenttype
 
 TRACE = False
@@ -237,6 +239,31 @@ def concat(mapping, *keys):
     return '\n'.join(values)
 
 
+
+def has_license_with_copyright(text):
+    """
+    Return True if the LegalCopyright `text` could have some license
+    declarations and should be a part of the extracted_license_statement.
+    """
+    copyrights = detect_copyrights_from_lines(
+        numbered_lines=[tuple([1, text])],
+        include_copyrights=True,
+        include_authors=False,
+        include_holders=False,
+        include_copyright_years=True,
+        include_copyright_allrights=True,
+    )
+    detections = [detection.to_dict() for detection in copyrights]
+    if text and not detections:
+        return True
+
+    if detections and "copyright" in detections[0]:
+        return False
+
+    return True
+
+
+
 class WindowsExecutableHandler(models.NonAssemblableDatafileHandler):
     datasource_id = 'windows_executable'
     default_package_type = 'winexe'
@@ -278,66 +305,70 @@ def is_datafile(cls, location, filetypes=tuple()):
     @classmethod
     def parse(cls, location, package_only=False):
         infos = pe_info(location)
+        yield get_package_data_from_pe_info(infos, package_only)
+
 
-        version = get_first(
-            infos,
-            'Full Version',
-            'ProductVersion',
-            'FileVersion',
-            'Assembly Version',
-        )
-        release_date = get_first(infos, 'BuildDate')
-        if release_date:
-            if len(release_date) >= 10:
-                release_date = release_date[:10]
-            release_date = release_date.replace('/', '-')
-
-        name = get_first(
-            infos,
-            'ProductName',
-            'OriginalFilename',
-            'InternalName',
-        )
-        copyr = get_first(infos, 'LegalCopyright')
-
-        LegalCopyright = copyr,
-
-        LegalTrademarks = concat(
-            infos,
-            'LegalTrademarks',
-            'LegalTrademarks1',
-            'LegalTrademarks2',
-            'LegalTrademarks3')
-
-        License = get_first(infos, 'License')
-
-        extracted_license_statement = None
-        if LegalTrademarks or License:
-            extracted_license_statement = {}
-            if LegalTrademarks and LegalTrademarks != '':
-                extracted_license_statement['LegalTrademarks'] = LegalTrademarks
-            if License:
-                extracted_license_statement['License'] = License
-
-        description = concat(infos, 'FileDescription', 'Comments')
-
-        parties = []
-        cname = get_first(infos, 'CompanyName', 'Company')
-
-        if cname:
-            parties = [Party(type=party_org, role='author', name=cname)]
-        homepage_url = get_first(infos, 'URL', 'WWW')
-
-        package_data = dict(
-            datasource_id=cls.datasource_id,
-            type=cls.default_package_type,
-            name=name,
-            version=version,
-            release_date=release_date,
-            copyright=copyr,
-            extracted_license_statement=extracted_license_statement,
-            description=description,
-            parties=parties,
-            homepage_url=homepage_url,
-        )
-        yield models.PackageData.from_data(package_data, package_only)
+def get_package_data_from_pe_info(infos, package_only=False):
+
+    version = get_first(
+        infos,
+        'Full Version',
+        'ProductVersion',
+        'FileVersion',
+        'Assembly Version',
+    )
+    release_date = get_first(infos, 'BuildDate')
+    if release_date:
+        if len(release_date) >= 10:
+            release_date = release_date[:10]
+        release_date = release_date.replace('/', '-')
+
+    name = get_first(
+        infos,
+        'ProductName',
+        'OriginalFilename',
+        'InternalName',
+    )
+
+    LegalCopyright = get_first(infos, 'LegalCopyright')
+    copyr_has_license = LegalCopyright and has_license_with_copyright(LegalCopyright)
+    LegalTrademarks = concat(
+        infos,
+        'LegalTrademarks',
+        'LegalTrademarks1',
+        'LegalTrademarks2',
+        'LegalTrademarks3')
+    License = get_first(infos, 'License')
+
+    extracted_license_statement = None
+    if copyr_has_license or LegalTrademarks or License:
+        extracted_license_statement = {}
+        if copyr_has_license:
+            extracted_license_statement['LegalCopyright'] = LegalCopyright
+        if LegalTrademarks and LegalTrademarks != '':
+            extracted_license_statement['LegalTrademarks'] = LegalTrademarks
+        if License:
+            extracted_license_statement['License'] = License
+
+    description = concat(infos, 'FileDescription', 'Comments')
+
+    parties = []
+    cname = get_first(infos, 'CompanyName', 'Company')
+
+    if cname:
+        parties = [Party(type=party_org, role='author', name=cname)]
+    homepage_url = get_first(infos, 'URL', 'WWW')
+
+    package_data = dict(
+        datasource_id=WindowsExecutableHandler.datasource_id,
+        type=WindowsExecutableHandler.default_package_type,
+        name=name,
+        version=version,
+        release_date=release_date,
+        copyright=LegalCopyright,
+        extracted_license_statement=extracted_license_statement,
+        description=description,
+        parties=parties,
+        homepage_url=homepage_url,
+    )
+    return models.PackageData.from_data(package_data, package_only)

src/summarycode/classify.py

@@ -94,6 +94,13 @@ def get_relative_path(root_path, path):
     'readme',
 )
 
+# Community files are usually files used for FOSS project and community
+# maintainence purposes. We want to detect these as in the context of
+# licenses as these files don't have interesting license detections, or
+# license detection issues are not important to review for these files.
+# this is similar to `key` files, which also has a lot of community info
+# but there the license declarations are extremely important as they have
+# information on the primary (or even secondary) licenses for the package
 COMMUNITY_FILES = (
     'CHANGELOG',
     'ROADMAP',
@@ -129,10 +136,16 @@ def check_is_community_file(filename):
 
 
 def check_is_resource_community_file(resource):
+    """
+    Return True if the `resource` is a community file.
+    """
     return check_is_community_file(resource.name) or check_is_community_file(resource.base_name)
 
 
 def check_is_path_community_file(path):
+    """
+    Return True if the file at `path` is a community file.
+    """
     name = file_name(path, force_posix=True)
     base_name = file_base_name(path, force_posix=True)
     return check_is_community_file(name) or check_is_community_file(base_name)
@@ -141,7 +154,7 @@ def check_is_path_community_file(path):
 def check_resource_name_start_and_end(resource, STARTS_ENDS):
     """
     Return True if `resource.name` or `resource.base_name` begins or ends with
-    an element of `STARTS_ENDS`
+    an element of `STARTS_ENDS`.
     """
     name = resource.name.lower()
     base_name = resource.base_name.lower()
@@ -159,7 +172,7 @@ def set_classification_flags(resource,
     _README=README_STARTS_ENDS,
 ):
     """
-    Set classification flags on the `resource` Resource
+    Set classification flags on the `resource` Resource.
     """
     path = resource.path.lower()
 

tests/packagedcode/data/win_pe/DockerPull.exe.expected.json

@@ -0,0 +1,23 @@
+{
+  "Full Version": null,
+  "ProductVersion": "1.0.7.0",
+  "FileVersion": "1.0.7.0",
+  "Assembly Version": null,
+  "BuildDate": null,
+  "ProductName": "DockerPull",
+  "OriginalFilename": "DockerPull.exe",
+  "InternalName": "DockerPull",
+  "License": null,
+  "LegalCopyright": "MIT License | GitHub: https://github.com/topcss/docker-pull-tar",
+  "LegalTrademarks": null,
+  "LegalTrademarks1": null,
+  "LegalTrademarks2": null,
+  "LegalTrademarks3": null,
+  "FileDescription": "Docker Image Puller \u65e0\u9700\u5b89\u88c5 Docker \u6216 Python \u73af\u5883,\u76f4\u63a5\u4ece Docker \u4ed3\u5e93\u62c9\u53d6\u955c\u50cf,\u652f\u6301\u56fd\u5185\u955c\u50cf\u6e90\u52a0\u901f\u548c\u591a\u67b6\u6784\u652f\u6301\u3002\u9879\u76ee\u5730\u5740:https://github.com/topcss/docker-pull-tar",
+  "Comments": null,
+  "CompanyName": "topcss",
+  "Company": null,
+  "URL": null,
+  "WWW": null,
+  "extra_data": {}
+}