From 171c4859f4cbe37d20d036e11a75857d78beccb4 Mon Sep 17 00:00:00 2001 From: Igor Opaniuk Date: Tue, 28 Apr 2026 08:47:14 +0200 Subject: [PATCH 1/3] gen_partition: extract main() and remove top-level execution The module ran argv parsing and XML emission as top-level statements at import time, so it could not be imported by pytest or any library caller. This blocks the follow-up work: unit-testing the parser and splitting it into a loaders package. Wrap the flow in main(argv=None) -> int gated by __name__ == "__main__", move the module-level accumulators into locals, and return status codes instead of sys.exit(). The runpy dispatcher in cli.py is unaffected because it sets __name__ to "__main__". Signed-off-by: Igor Opaniuk --- qcom_ptool/gen_partition.py | 119 ++++++++++++++++++++---------------- 1 file changed, 66 insertions(+), 53 deletions(-) diff --git a/qcom_ptool/gen_partition.py b/qcom_ptool/gen_partition.py index b6449d0..03388d0 100755 --- a/qcom_ptool/gen_partition.py +++ b/qcom_ptool/gen_partition.py @@ -27,6 +27,8 @@ # OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN # IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +from __future__ import annotations + import getopt import re import sys @@ -69,13 +71,9 @@ def usage() -> NoReturn: } ################################################################## -# store entries read from input file -disk_entry = None -partition_entries = [] -# store partition image map passed from command line +# Store partition image map passed from command line. Populated by main() +# and read by partition_options() during parsing of each --partition line. partition_image_map: dict[str, str] = {} -input_file = None -output_xml = None def disk_options(argv): @@ -236,58 +234,73 @@ def generate_partition_xml(disk_params, partitions, output_xml): ############################################################################### # main -disk_entry_err_msg = "contains more than one --disk entries" -if len(sys.argv) < 3: - usage() -try: - if sys.argv[1] == "-h" or sys.argv[1] == "--help": - usage() - try: - opts, rem = getopt.getopt(sys.argv[1:], "i:o:m:") - for opt, arg in opts: - if opt in ["-i"]: - input_file = arg - elif opt in ["-o"]: - output_xml = arg - elif opt in ["-m"]: - for mapping in arg.split(","): - tags = mapping.split("=") - if len(tags) > 1: - partition_image_map[tags[0]] = tags[1] - else: - usage() +def main(argv: list[str] | None = None) -> int: + if argv is None: + argv = sys.argv - except Exception as argerr: - print(str(argerr)) - usage() - if input_file is None or output_xml is None: + disk_entry_err_msg = "contains more than one --disk entries" + + if len(argv) < 3: usage() - f = open(input_file) - line = f.readline() - while line: - if not re.search(r"^\s*#", line) and not re.search(r"^\s*$", line): - line = line.strip() - if re.search("^--disk", line): - if disk_entry is None: - disk_entry = line + + input_file: str | None = None + output_xml: str | None = None + disk_entry: str | None = None + partition_entries: list[str] = [] + partition_image_map.clear() + + try: + if argv[1] == "-h" or argv[1] == "--help": + usage() + try: + opts, _rem = getopt.getopt(argv[1:], "i:o:m:") + for opt, arg in opts: + if opt in ["-i"]: + input_file = arg + elif opt in ["-o"]: + output_xml = arg + elif opt in ["-m"]: + for mapping in arg.split(","): + tags = mapping.split("=") + if len(tags) > 1: + partition_image_map[tags[0]] = tags[1] else: - print("%s %s" % (sys.argv[1], disk_entry_err_msg)) - print("%s\n%s" % (disk_entry, line)) - sys.exit(1) - elif re.search("^--partition", line): - partition_entries.append(line) - else: - print("Ignoring %s" % (line)) + usage() + + except Exception as argerr: + print(str(argerr)) + usage() + if input_file is None or output_xml is None: + usage() + f = open(input_file) line = f.readline() - f.close() -except Exception as e: - print("Error: ", e) - sys.exit(1) + while line: + if not re.search(r"^\s*#", line) and not re.search(r"^\s*$", line): + line = line.strip() + if re.search("^--disk", line): + if disk_entry is None: + disk_entry = line + else: + print("%s %s" % (argv[1], disk_entry_err_msg)) + print("%s\n%s" % (disk_entry, line)) + return 1 + elif re.search("^--partition", line): + partition_entries.append(line) + else: + print("Ignoring %s" % (line)) + line = f.readline() + f.close() + except Exception as e: + print("Error: ", e) + return 1 + + disk_params = parse_disk_entry(disk_entry) + partitions = parse_partition_entries(partition_entries) + generate_partition_xml(disk_params, partitions, output_xml) + return 0 -disk_params = parse_disk_entry(disk_entry) -partitions = parse_partition_entries(partition_entries) -generate_partition_xml(disk_params, partitions, output_xml) -sys.exit(0) +if __name__ == "__main__": + sys.exit(main()) From 9959439fffca1d67a0c02d301b4fd48e9d5a2b4d Mon Sep 17 00:00:00 2001 From: Igor Opaniuk Date: Tue, 28 Apr 2026 08:49:08 +0200 Subject: [PATCH 2/3] qcom_ptool: decouple input-file parsing into a loaders package The .conf format was hard-coded into gen_partition.py, with no boundary between where data comes from and how partition XML is emitted. Adding another format (YAML) would mean duplicating main() or scattering format branches through every helper. Add qcom_ptool/spec.py for the canonical internal shape (DiskParams, PartitionEntry, PartitionsByLun, LoadedSpec) and qcom_ptool/loaders/ with a load(path, image_map) dispatcher keyed on file extension: a new format becomes a new module plus a suffix registration. Move the conf parser into loaders/conf.py and apply the image-map override in one post-load pass, removing the last shared-global coupling. Signed-off-by: Igor Opaniuk --- qcom_ptool/gen_partition.py | 233 +++++---------------------------- qcom_ptool/loaders/__init__.py | 40 ++++++ qcom_ptool/loaders/conf.py | 219 +++++++++++++++++++++++++++++++ qcom_ptool/spec.py | 73 +++++++++++ 4 files changed, 366 insertions(+), 199 deletions(-) create mode 100644 qcom_ptool/loaders/__init__.py create mode 100644 qcom_ptool/loaders/conf.py create mode 100644 qcom_ptool/spec.py diff --git a/qcom_ptool/gen_partition.py b/qcom_ptool/gen_partition.py index 03388d0..b835220 100755 --- a/qcom_ptool/gen_partition.py +++ b/qcom_ptool/gen_partition.py @@ -30,13 +30,14 @@ from __future__ import annotations import getopt -import re import sys import xml.etree.ElementTree as ET -from collections import OrderedDict from typing import NoReturn from xml.dom import minidom +from qcom_ptool.loaders import load as load_spec +from qcom_ptool.spec import DiskParams, PartitionsByLun + def usage() -> NoReturn: print( @@ -46,155 +47,9 @@ def usage() -> NoReturn: sys.exit(1) -################################################################## -# defaults to be used -disk_params_defaults = OrderedDict( - { - "type": "", - "size": "", - "SECTOR_SIZE_IN_BYTES": "512", - "WRITE_PROTECT_BOUNDARY_IN_KB": "65536", - "GROW_LAST_PARTITION_TO_FILL_DISK": "false", - "ALIGN_PARTITIONS_TO_PERFORMANCE_BOUNDARY": "true", - "PERFORMANCE_BOUNDARY_IN_KB": "4", - } -) - -partition_entry_defaults = { - "label": "", - "size_in_kb": "", - "type": "00000000-0000-0000-0000-000000000000", - "bootable": "false", - "readonly": "true", - "filename": "", - "sparse": "false", -} - -################################################################## -# Store partition image map passed from command line. Populated by main() -# and read by partition_options() during parsing of each --partition line. -partition_image_map: dict[str, str] = {} - - -def disk_options(argv): - disk_params = disk_params_defaults.copy() - for opt, arg in argv: - if opt in ["--type"]: - disk_params["type"] = arg - elif opt in ["--size"]: - disk_params["size"] = arg - elif opt in ["--sector-size-in-bytes"]: - disk_params["SECTOR_SIZE_IN_BYTES"] = arg - elif opt in ["--write-protect-boundary"]: - disk_params["WRITE_PROTECT_BOUNDARY_IN_KB"] = arg - elif opt in ["--grow-last-partition"]: - disk_params["GROW_LAST_PARTITION_TO_FILL_DISK"] = "true" - elif opt in ["--align-partitions"]: - disk_params["ALIGN_PARTITIONS_TO_PERFORMANCE_BOUNDARY"] = "true" - disk_params["PERFORMANCE_BOUNDARY_IN_KB"] = str(int(arg) // 1024) - return disk_params - - -def partition_size_in_kb(size): - if not re.search("[a-zA-Z]+", size): - return int(size) // 1024 - m = re.search("([0-9]+)(?=[Kk][Bb]?)", size) - if m: - return int(m.group(0)) - m = re.search("([0-9]+)(?=[Mm][Bb]?)", size) - if m: - return int(m.group(0)) * 1024 - m = re.search("([0-9]+)(?=[Gg][Bb]?)", size) - if m: - return int(m.group(0)) * 1024 * 1024 - raise ValueError("Unrecognized size format: '%s'" % size) - - -def partition_options(argv): - partition_entry = partition_entry_defaults.copy() - phys_part = 0 - for opt, arg in argv: - if opt in ["--lun", "--phys-part"]: - phys_part = arg - elif opt in ["--name"]: - partition_entry["label"] = arg - elif opt in ["--size"]: - kbytes = partition_size_in_kb(arg) - partition_entry["size_in_kb"] = str(kbytes) - elif opt in ["--type-guid"]: - partition_entry["type"] = arg - elif opt in ["--attributes"]: - attribute_bits = int(arg, 16) - if attribute_bits & (1 << 2): - partition_entry["bootable"] = "true" - else: - partition_entry["bootable"] = "false" - if attribute_bits & (1 << 60): - partition_entry["readonly"] = "true" - else: - partition_entry["readonly"] = "false" - elif opt in ["--filename"]: - partition_entry["filename"] = arg - elif opt in ["--sparse"]: - partition_entry["sparse"] = arg - if partition_entry["label"] in partition_image_map: - partition_entry["filename"] = partition_image_map[partition_entry["label"]] - return phys_part, partition_entry - - -def parse_partition_entries(partition_entries): - partitions_params: dict[int, list[dict[str, str]]] = {} - - for partition_entry in partition_entries: - opts_list = list(partition_entry.split(" ")) - if opts_list[0] == "--partition": - try: - options, _remainders = getopt.gnu_getopt( - opts_list[1:], - "", - [ - "lun=", - "phys-part=", - "name=", - "size=", - "type-guid=", - "filename=", - "attributes=", - "sparse=", - ], - ) - phys_part, partition = partition_options(options) - partitions_params.setdefault(phys_part, []).append(partition) - except Exception as e: - print(str(e)) - usage() - - return partitions_params - - -def parse_disk_entry(disk_entry): - opts_list = list(disk_entry.split(" ")) - if opts_list[0] == "--disk": - try: - options, _remainders = getopt.gnu_getopt( - opts_list[1:], - "", - [ - "type=", - "size=", - "sector-size-in-bytes=", - "write-protect-boundary=", - "grow-last-partition", - "align-partitions=", - ], - ) - return disk_options(options) - except Exception as e: - print(str(e)) - usage() - - -def generate_multi_lun_xml(disk_params, partitions, output_xml): +def generate_multi_lun_xml( + disk_params: DiskParams, partitions: PartitionsByLun, output_xml: str +) -> None: root = ET.Element("configuration") parser_instruction_text = "" @@ -220,7 +75,9 @@ def generate_multi_lun_xml(disk_params, partitions, output_xml): f.write(xmlstr) -def generate_partition_xml(disk_params, partitions, output_xml): +def generate_partition_xml( + disk_params: DiskParams, partitions: PartitionsByLun, output_xml: str +) -> None: print("Generating %s XML %s" % (disk_params["type"].upper(), output_xml)) if disk_params["type"] in ("emmc", "nvme", "spinor", "ufs"): @@ -240,65 +97,43 @@ def main(argv: list[str] | None = None) -> int: if argv is None: argv = sys.argv - disk_entry_err_msg = "contains more than one --disk entries" - if len(argv) < 3: usage() input_file: str | None = None output_xml: str | None = None - disk_entry: str | None = None - partition_entries: list[str] = [] - partition_image_map.clear() + image_map: dict[str, str] = {} + if argv[1] == "-h" or argv[1] == "--help": + usage() try: - if argv[1] == "-h" or argv[1] == "--help": - usage() - try: - opts, _rem = getopt.getopt(argv[1:], "i:o:m:") - for opt, arg in opts: - if opt in ["-i"]: - input_file = arg - elif opt in ["-o"]: - output_xml = arg - elif opt in ["-m"]: - for mapping in arg.split(","): - tags = mapping.split("=") - if len(tags) > 1: - partition_image_map[tags[0]] = tags[1] - else: - usage() + opts, _rem = getopt.getopt(argv[1:], "i:o:m:") + for opt, arg in opts: + if opt == "-i": + input_file = arg + elif opt == "-o": + output_xml = arg + elif opt == "-m": + for mapping in arg.split(","): + tags = mapping.split("=") + if len(tags) > 1: + image_map[tags[0]] = tags[1] + else: + usage() + except Exception as argerr: + print(str(argerr)) + usage() - except Exception as argerr: - print(str(argerr)) - usage() - if input_file is None or output_xml is None: - usage() - f = open(input_file) - line = f.readline() - while line: - if not re.search(r"^\s*#", line) and not re.search(r"^\s*$", line): - line = line.strip() - if re.search("^--disk", line): - if disk_entry is None: - disk_entry = line - else: - print("%s %s" % (argv[1], disk_entry_err_msg)) - print("%s\n%s" % (disk_entry, line)) - return 1 - elif re.search("^--partition", line): - partition_entries.append(line) - else: - print("Ignoring %s" % (line)) - line = f.readline() - f.close() + if input_file is None or output_xml is None: + usage() + + try: + spec = load_spec(input_file, image_map=image_map) except Exception as e: print("Error: ", e) return 1 - disk_params = parse_disk_entry(disk_entry) - partitions = parse_partition_entries(partition_entries) - generate_partition_xml(disk_params, partitions, output_xml) + generate_partition_xml(spec["disk"], spec["partitions"], output_xml) return 0 diff --git a/qcom_ptool/loaders/__init__.py b/qcom_ptool/loaders/__init__.py new file mode 100644 index 0000000..30a4bf0 --- /dev/null +++ b/qcom_ptool/loaders/__init__.py @@ -0,0 +1,40 @@ +# Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. +# SPDX-License-Identifier: BSD-3-Clause +""" +Source-format loaders for partition specs. + +Each loader exposes ``load(path, image_map=None) -> LoadedSpec`` so the rest +of the package never has to care which on-disk format produced the +:mod:`qcom_ptool.spec` representation it operates on. Adding a new format +(e.g. YAML) means dropping a new module here and registering its suffix in +the dispatcher below — no other module needs to change. +""" + +from __future__ import annotations + +import os +from collections.abc import Mapping + +from qcom_ptool.spec import LoadedSpec + + +class UnsupportedFormatError(ValueError): + """Raised when no loader is registered for a given path's extension.""" + + +def load(path: str, image_map: Mapping[str, str] | None = None) -> LoadedSpec: + """Dispatch to the appropriate loader based on file extension. + + ``image_map`` overrides the ``--filename`` (or equivalent) for partitions + whose name appears as a key. It's applied uniformly by every loader so + the CLI ``-m`` flag works the same regardless of source format. + """ + suffix = os.path.splitext(path)[1].lower() + if suffix in ("", ".conf"): + # Late import: keeps this dispatcher dependency-free until a format + # is actually requested, which matters once optional formats (YAML) + # land with their own third-party imports. + from qcom_ptool.loaders import conf + + return conf.load(path, image_map=image_map) + raise UnsupportedFormatError(f"No loader registered for suffix {suffix!r}") diff --git a/qcom_ptool/loaders/conf.py b/qcom_ptool/loaders/conf.py new file mode 100644 index 0000000..9c905e1 --- /dev/null +++ b/qcom_ptool/loaders/conf.py @@ -0,0 +1,219 @@ +# Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. +# SPDX-License-Identifier: BSD-3-Clause +""" +Loader for the legacy ``--disk`` / ``--partition`` line-format files under +``platforms/``. + +A ``.conf`` file is an unstructured stream of comment lines, blank lines, +exactly one ``--disk`` line, and any number of ``--partition`` lines. Each +significant line is itself a getopt-style option list. This module parses +both layers and returns a :class:`qcom_ptool.spec.LoadedSpec`. + +The parsing helpers are kept individually addressable so the unit tests +under ``tests/unit/`` can characterise each layer in isolation. +""" + +from __future__ import annotations + +import getopt +import re +from collections.abc import Mapping + +from qcom_ptool.spec import ( + DISK_PARAMS_DEFAULTS, + PARTITION_ENTRY_DEFAULTS, + DiskParams, + LoadedSpec, + PartitionEntry, + PartitionsByLun, +) + + +class ConfParseError(ValueError): + """Raised when a ``.conf`` file cannot be parsed.""" + + +# --------------------------------------------------------------------------- +# Size string parsing +# --------------------------------------------------------------------------- + + +def partition_size_in_kb(size: str) -> int: + """Convert a size string ("1024", "1KB", "2MB", "1GB") to KB. + + Bare integers are interpreted as bytes and divided by 1024. Strings with a + K/M/G suffix (any case, with optional 'B') return the kilobyte equivalent. + Anything else raises ``ValueError``; callers may catch and reformat. + """ + if not re.search("[a-zA-Z]+", size): + return int(size) // 1024 + m = re.search("([0-9]+)(?=[Kk][Bb]?)", size) + if m: + return int(m.group(0)) + m = re.search("([0-9]+)(?=[Mm][Bb]?)", size) + if m: + return int(m.group(0)) * 1024 + m = re.search("([0-9]+)(?=[Gg][Bb]?)", size) + if m: + return int(m.group(0)) * 1024 * 1024 + raise ValueError("Unrecognized size format: '%s'" % size) + + +# --------------------------------------------------------------------------- +# Option-list -> normalised dict +# --------------------------------------------------------------------------- + + +def disk_options(argv: list[tuple[str, str]]) -> DiskParams: + """Translate parsed ``--disk`` options into the canonical disk dict.""" + disk = DISK_PARAMS_DEFAULTS.copy() + for opt, arg in argv: + if opt == "--type": + disk["type"] = arg + elif opt == "--size": + disk["size"] = arg + elif opt == "--sector-size-in-bytes": + disk["SECTOR_SIZE_IN_BYTES"] = arg + elif opt == "--write-protect-boundary": + disk["WRITE_PROTECT_BOUNDARY_IN_KB"] = arg + elif opt == "--grow-last-partition": + disk["GROW_LAST_PARTITION_TO_FILL_DISK"] = "true" + elif opt == "--align-partitions": + disk["ALIGN_PARTITIONS_TO_PERFORMANCE_BOUNDARY"] = "true" + disk["PERFORMANCE_BOUNDARY_IN_KB"] = str(int(arg) // 1024) + return disk + + +def partition_options( + argv: list[tuple[str, str]], + image_map: Mapping[str, str] | None = None, +) -> tuple[str, PartitionEntry]: + """Translate parsed ``--partition`` options into ``(phys_part, entry)``. + + ``image_map`` (if provided) overrides the entry's ``filename`` when the + partition name matches a key. The override is applied after all options + have been processed, so it always wins over an explicit ``--filename``. + """ + if image_map is None: + image_map = {} + entry: PartitionEntry = PARTITION_ENTRY_DEFAULTS.copy() + phys_part: str = "0" + for opt, arg in argv: + if opt in ("--lun", "--phys-part"): + phys_part = arg + elif opt == "--name": + entry["label"] = arg + elif opt == "--size": + entry["size_in_kb"] = str(partition_size_in_kb(arg)) + elif opt == "--type-guid": + entry["type"] = arg + elif opt == "--attributes": + attribute_bits = int(arg, 16) + entry["bootable"] = "true" if attribute_bits & (1 << 2) else "false" + entry["readonly"] = "true" if attribute_bits & (1 << 60) else "false" + elif opt == "--filename": + entry["filename"] = arg + elif opt == "--sparse": + entry["sparse"] = arg + if entry["label"] in image_map: + entry["filename"] = image_map[entry["label"]] + return phys_part, entry + + +# --------------------------------------------------------------------------- +# Line-level parsing +# --------------------------------------------------------------------------- + + +_DISK_LONG_OPTS = [ + "type=", + "size=", + "sector-size-in-bytes=", + "write-protect-boundary=", + "grow-last-partition", + "align-partitions=", +] + +_PARTITION_LONG_OPTS = [ + "lun=", + "phys-part=", + "name=", + "size=", + "type-guid=", + "filename=", + "attributes=", + "sparse=", +] + + +def parse_disk_line(line: str) -> DiskParams | None: + """Parse a single ``--disk ...`` line; return None if the line isn't one.""" + opts_list = line.split(" ") + if not opts_list or opts_list[0] != "--disk": + return None + options, _rem = getopt.gnu_getopt(opts_list[1:], "", _DISK_LONG_OPTS) + return disk_options(options) + + +def parse_partition_lines( + lines: list[str], + image_map: Mapping[str, str] | None = None, +) -> PartitionsByLun: + """Parse a list of ``--partition ...`` lines, grouped by LUN/phys-part.""" + if image_map is None: + image_map = {} + partitions: PartitionsByLun = {} + for line in lines: + opts_list = line.split(" ") + if not opts_list or opts_list[0] != "--partition": + continue + options, _rem = getopt.gnu_getopt(opts_list[1:], "", _PARTITION_LONG_OPTS) + phys_part, entry = partition_options(options, image_map) + partitions.setdefault(phys_part, []).append(entry) + return partitions + + +# --------------------------------------------------------------------------- +# File-level parsing +# --------------------------------------------------------------------------- + + +def read_conf(path: str) -> tuple[str, list[str]]: + """Read ``path`` and return ``(disk_line, [partition_lines])``. + + Comments (``#``-prefixed) and blank lines are skipped. Multiple ``--disk`` + lines are an error. Lines that aren't ``--disk`` or ``--partition`` are + printed as "Ignoring ..." for parity with the original behaviour. + """ + disk_line: str | None = None + partition_lines: list[str] = [] + with open(path) as f: + for raw in f: + if re.search(r"^\s*#", raw) or re.search(r"^\s*$", raw): + continue + line = raw.strip() + if line.startswith("--disk"): + if disk_line is not None: + raise ConfParseError( + "%s contains more than one --disk entries:\n%s\n%s" + % (path, disk_line, line) + ) + disk_line = line + elif line.startswith("--partition"): + partition_lines.append(line) + else: + print("Ignoring %s" % line) + if disk_line is None: + raise ConfParseError("%s contains no --disk entry" % path) + return disk_line, partition_lines + + +def load(path: str, image_map: Mapping[str, str] | None = None) -> LoadedSpec: + """Public entry point: read ``path`` and return a normalised ``LoadedSpec``.""" + disk_line, partition_lines = read_conf(path) + disk = parse_disk_line(disk_line) + if disk is None: + # Should be unreachable: read_conf guarantees disk_line starts with --disk. + raise ConfParseError("%s: failed to parse --disk line" % path) + partitions = parse_partition_lines(partition_lines, image_map) + return {"disk": disk, "partitions": partitions} diff --git a/qcom_ptool/spec.py b/qcom_ptool/spec.py new file mode 100644 index 0000000..580bf54 --- /dev/null +++ b/qcom_ptool/spec.py @@ -0,0 +1,73 @@ +# Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. +# SPDX-License-Identifier: BSD-3-Clause +""" +Internal representation shared by all input loaders. + +Every loader (``loaders/conf.py`` today, ``loaders/yaml.py`` tomorrow) must +return a ``LoadedSpec`` so the rest of ``gen_partition.py`` is independent +of the source format. + +``DiskParams`` and ``PartitionEntry`` are type aliases for ``dict[str, str]`` +rather than ``TypedDict``s; the XML emitter passes the dicts straight to +``ET.SubElement(..., attrib=...)`` which requires a plain ``dict[str, str]`` +at runtime, and aliasing keeps that contract precise without TypedDict's +``total=False`` ``object`` widening. Migrating to dataclasses (with explicit +``to_attrib()`` methods) is a clean follow-up if stronger validation becomes +worth its blast radius. +""" + +from __future__ import annotations + +from collections import OrderedDict +from typing import TypedDict + +# Parsed ``--disk`` line normalised into the keys ``ptool`` consumes. +# Expected keys: type, size, SECTOR_SIZE_IN_BYTES, WRITE_PROTECT_BOUNDARY_IN_KB, +# GROW_LAST_PARTITION_TO_FILL_DISK, ALIGN_PARTITIONS_TO_PERFORMANCE_BOUNDARY, +# PERFORMANCE_BOUNDARY_IN_KB. See DISK_PARAMS_DEFAULTS for the canonical set. +DiskParams = dict[str, str] + +# Parsed ``--partition`` line normalised for the XML attribute set. +# Expected keys: label, size_in_kb, type, bootable, readonly, filename, sparse. +# See PARTITION_ENTRY_DEFAULTS for the canonical set. +PartitionEntry = dict[str, str] + +# Mapping of physical partition / LUN id (kept as a string for backward +# compatibility with the existing line parser) to its partition entries. +PartitionsByLun = dict[str, list[PartitionEntry]] + + +# Defaults used by the conf loader (and any future loader that wants to +# inherit the same baseline). Kept as module-level constants so loaders can +# ``.copy()`` from them rather than rebuilding the structure each call. +# ``OrderedDict`` is preserved here because the XML emitter relies on its +# iteration order to produce stable output across Python versions. +DISK_PARAMS_DEFAULTS: DiskParams = OrderedDict( + { + "type": "", + "size": "", + "SECTOR_SIZE_IN_BYTES": "512", + "WRITE_PROTECT_BOUNDARY_IN_KB": "65536", + "GROW_LAST_PARTITION_TO_FILL_DISK": "false", + "ALIGN_PARTITIONS_TO_PERFORMANCE_BOUNDARY": "true", + "PERFORMANCE_BOUNDARY_IN_KB": "4", + } +) + + +PARTITION_ENTRY_DEFAULTS: PartitionEntry = { + "label": "", + "size_in_kb": "", + "type": "00000000-0000-0000-0000-000000000000", + "bootable": "false", + "readonly": "true", + "filename": "", + "sparse": "false", +} + + +class LoadedSpec(TypedDict): + """What every loader returns from ``load(path)``.""" + + disk: DiskParams + partitions: PartitionsByLun From 5aad8a964de6a975376b6262921f93c44da76a4e Mon Sep 17 00:00:00 2001 From: Igor Opaniuk Date: Tue, 28 Apr 2026 10:40:31 +0200 Subject: [PATCH 3/3] tests: add pytest unit suite for the .conf loader make integration only checks that referenced files exist; it does not cover size parsing, attribute-bit decoding, LUN grouping or the image-map override -- the contract any future loader must reproduce. Add tests/unit/ pinning those cases against conf.load() and the loaders dispatcher. Wire it in with a make unit-test target, add it to make check, install python3-pytest in CI, and keep pytest config in pyproject.toml so pytest runs from the repo root. Signed-off-by: Igor Opaniuk --- .github/workflows/build.yml | 5 +- Makefile | 7 +- README.md | 20 +- pyproject.toml | 7 + tests/unit/__init__.py | 0 tests/unit/test_loaders_conf.py | 381 ++++++++++++++++++++++++++++++++ 6 files changed, 407 insertions(+), 13 deletions(-) create mode 100644 tests/unit/__init__.py create mode 100644 tests/unit/test_loaders_conf.py diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index aee107e..38abae8 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -14,11 +14,11 @@ jobs: with: fetch-depth: 0 - - name: Install linters + - name: Install linters and test runner run: | sudo snap install ruff sudo apt-get update - sudo apt-get install -y mypy + sudo apt-get install -y mypy python3-pytest - name: Install qcom-ptool run: | @@ -29,6 +29,7 @@ jobs: PTOOL_SEED: qcom-ptool-ci run: | make lint + make unit-test make all integration check-checksums - name: Verify checksum manifest is up to date diff --git a/Makefile b/Makefile index a6a6d54..453e3d8 100644 --- a/Makefile +++ b/Makefile @@ -12,7 +12,7 @@ QCOM_PTOOL ?= qcom-ptool # optional build_id for Axiom contents.xml files BUILD_ID ?= -.PHONY: all check check-checksums clean generate-checksums install lint integration +.PHONY: all check check-checksums clean generate-checksums install lint integration unit-test all: $(PLATFORMS) $(PARTITIONS_XML) $(CONTENTS_XML) @@ -29,6 +29,9 @@ lint: ruff check qcom_ptool mypy qcom_ptool +unit-test: + pytest + integration: all # make sure generated output has created expected files tests/integration/check-missing-files platforms/*/*/*.xml @@ -46,7 +49,7 @@ generate-checksums: all ! -name '*.xml.in' -print0 | LC_ALL=C sort -z | xargs -0 sha256sum \ > tests/integration/checksums.sha256 -check: lint integration +check: lint unit-test integration install: pip install . diff --git a/README.md b/README.md index 4bed9c8..11e1fc6 100644 --- a/README.md +++ b/README.md @@ -33,14 +33,14 @@ subcommand. At runtime, the scripts use only the Python standard library (Python 3.8+), so no runtime dependencies need to be installed beyond the package itself. -For development, `make lint` invokes `ruff` and `mypy` directly from the -command line. On Debian/Ubuntu, install them as follows (ruff is not -packaged in apt on all releases/architectures, so we install it from -snap): +For development, `make lint` invokes `ruff` and `mypy` and `make unit-test` +runs the `pytest` suite under `tests/unit/`. On Debian/Ubuntu, install +them as follows (ruff is not packaged in apt on all releases/architectures, +so we install it from snap): ```sh sudo snap install ruff -sudo apt install mypy +sudo apt install mypy python3-pytest ``` ## Makefile targets @@ -49,8 +49,9 @@ sudo apt install mypy |---------------|------------------------------------------------------------| | `all` | Generate partition XML and GPT binaries for all platforms | | `lint` | Run ruff (linter) and mypy (type checker) on the package | +| `unit-test` | Run the pytest suite under `tests/unit/` | | `integration` | Build all platforms and verify generated files are present | -| `check` | Run both `lint` and `integration` | +| `check` | Run `lint`, `unit-test`, and `integration` | | `install` | Install the package (`pip install .`) | | `clean` | Remove generated XML and binary files from platforms/ | @@ -63,12 +64,13 @@ The Makefile invokes `qcom-ptool` from `PATH`. Install the package (or # install the tool pip install -e . -# install linters (Debian/Ubuntu) +# install linters and test runner (Debian/Ubuntu) sudo snap install ruff -sudo apt install mypy +sudo apt install mypy python3-pytest -# run linters +# run linters and unit tests make lint +make unit-test # build all platforms and run tests make check diff --git a/pyproject.toml b/pyproject.toml index 82b97cd..09a9710 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -53,3 +53,10 @@ ignore_missing_imports = true module = ["qcom_ptool.msp", "qcom_ptool.ptool"] # Legacy scripts - enable gradually check_untyped_defs = false + +[tool.pytest.ini_options] +# Allow running `pytest` from the repo root without requiring an editable +# install of the package; tests under tests/unit/ import qcom_ptool directly +# from the source tree. +pythonpath = ["."] +testpaths = ["tests/unit"] diff --git a/tests/unit/__init__.py b/tests/unit/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/unit/test_loaders_conf.py b/tests/unit/test_loaders_conf.py new file mode 100644 index 0000000..a5e02a6 --- /dev/null +++ b/tests/unit/test_loaders_conf.py @@ -0,0 +1,381 @@ +# Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. +# SPDX-License-Identifier: BSD-3-Clause +""" +Tests for ``qcom_ptool.loaders.conf`` plus a small main() smoke test. + +The bulk of these started life as characterization tests against the inline +parser in ``gen_partition.py`` and were rewritten to target the extracted +loader. They pin the contract that any future loader (e.g. YAML) must match +to remain byte-for-byte compatible with the .conf path. +""" + +from __future__ import annotations + +import pytest + +from qcom_ptool import gen_partition as gp +from qcom_ptool import spec +from qcom_ptool.loaders import UnsupportedFormatError, conf, load + + +# --------------------------------------------------------------------------- +# partition_size_in_kb +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "size,expected", + [ + # bare bytes -> divided by 1024 + ("1024", 1), + ("2048", 2), + ("0", 0), + # KB / Kb / Kk + ("1KB", 1), + ("128KB", 128), + ("4Kb", 4), + ("4K", 4), + # MB / Mb / M -> *1024 + ("1MB", 1024), + ("2MB", 2048), + ("4M", 4096), + # GB / Gb / G -> *1024*1024 + ("1GB", 1024 * 1024), + ("64GB", 64 * 1024 * 1024), + # mixed-prefix strings: regex returns the first numeric run before the suffix + ("foo123KB", 123), + ], +) +def test_partition_size_in_kb_recognised_forms(size: str, expected: int) -> None: + assert conf.partition_size_in_kb(size) == expected + + +@pytest.mark.parametrize("size", ["abc", "1TB", "MB", "1PB"]) +def test_partition_size_in_kb_unrecognised_suffix_raises(size: str) -> None: + """Strings with letters that don't match K/M/G hit the explicit raise.""" + with pytest.raises(ValueError, match="Unrecognized size format"): + conf.partition_size_in_kb(size) + + +def test_partition_size_in_kb_empty_string_raises_int_error() -> None: + """Empty strings take the "no letters -> int(size)" branch and surface + int()'s native ValueError rather than the explicit "Unrecognized" message. + Pinning this asymmetry so any future loader normalises empty strings + consistently.""" + with pytest.raises(ValueError, match="invalid literal for int"): + conf.partition_size_in_kb("") + + +# --------------------------------------------------------------------------- +# disk_options +# --------------------------------------------------------------------------- + + +def test_disk_options_returns_defaults_when_no_options() -> None: + result = conf.disk_options([]) + assert result == spec.DISK_PARAMS_DEFAULTS + # ensure caller receives a copy, not the shared default + assert result is not spec.DISK_PARAMS_DEFAULTS + + +def test_disk_options_basic_ufs() -> None: + opts = [ + ("--type", "ufs"), + ("--size", "137438953472"), + ("--sector-size-in-bytes", "4096"), + ("--write-protect-boundary", "0"), + ("--grow-last-partition", ""), + ] + result = conf.disk_options(opts) + assert result["type"] == "ufs" + assert result["size"] == "137438953472" + assert result["SECTOR_SIZE_IN_BYTES"] == "4096" + assert result["WRITE_PROTECT_BOUNDARY_IN_KB"] == "0" + assert result["GROW_LAST_PARTITION_TO_FILL_DISK"] == "true" + + +def test_disk_options_align_partitions_converts_bytes_to_kb() -> None: + # --align-partitions takes a value in bytes; stored value is in KB + result = conf.disk_options([("--align-partitions", "4096")]) + assert result["ALIGN_PARTITIONS_TO_PERFORMANCE_BOUNDARY"] == "true" + assert result["PERFORMANCE_BOUNDARY_IN_KB"] == "4" + + +def test_disk_options_unknown_flag_silently_ignored() -> None: + # The function intentionally only matches known flags; unknown flags + # leave the params at their defaults. + result = conf.disk_options([("--bogus", "x")]) + assert result == spec.DISK_PARAMS_DEFAULTS + + +# --------------------------------------------------------------------------- +# partition_options +# --------------------------------------------------------------------------- + + +def test_partition_options_basic() -> None: + opts = [ + ("--lun", "0"), + ("--name", "rootfs"), + ("--size", "33554432KB"), + ("--type-guid", "B921B045-1DF0-41C3-AF44-4C6F280D3FAE"), + ("--filename", "rootfs.img"), + ] + phys_part, entry = conf.partition_options(opts) + assert phys_part == "0" + assert entry["label"] == "rootfs" + assert entry["size_in_kb"] == "33554432" + assert entry["type"] == "B921B045-1DF0-41C3-AF44-4C6F280D3FAE" + assert entry["filename"] == "rootfs.img" + + +def test_partition_options_phys_part_alias() -> None: + # --phys-part and --lun are equivalent + _, entry = conf.partition_options([("--name", "x"), ("--phys-part", "3")]) + assert entry["label"] == "x" + phys_part, _ = conf.partition_options([("--phys-part", "3")]) + assert phys_part == "3" + + +def test_partition_options_size_with_suffix_normalises_to_kb() -> None: + _, entry = conf.partition_options([("--size", "2MB")]) + assert entry["size_in_kb"] == "2048" + + _, entry = conf.partition_options([("--size", "1GB")]) + assert entry["size_in_kb"] == str(1024 * 1024) + + +def test_partition_options_attributes_bootable_and_readonly_bits() -> None: + # bit 2 (0x4) -> bootable=true; bit 60 (1<<60) -> readonly=true + _, entry = conf.partition_options([("--attributes", "1000000000000004")]) + assert entry["bootable"] == "true" + assert entry["readonly"] == "true" + + # bit 2 alone -> bootable=true, readonly=false + _, entry = conf.partition_options([("--attributes", "4")]) + assert entry["bootable"] == "true" + assert entry["readonly"] == "false" + + # bit 60 alone -> bootable=false, readonly=true + _, entry = conf.partition_options([("--attributes", "1000000000000000")]) + assert entry["bootable"] == "false" + assert entry["readonly"] == "true" + + # neither bit -> bootable=false, readonly=false + _, entry = conf.partition_options([("--attributes", "0")]) + assert entry["bootable"] == "false" + assert entry["readonly"] == "false" + + +def test_partition_options_image_map_overrides_filename() -> None: + _, entry = conf.partition_options( + [("--name", "rootfs"), ("--filename", "default.img")], + image_map={"rootfs": "custom-rootfs.img"}, + ) + assert entry["filename"] == "custom-rootfs.img" + + +def test_partition_options_image_map_no_match_keeps_filename() -> None: + _, entry = conf.partition_options( + [("--name", "rootfs"), ("--filename", "default.img")], + image_map={"other": "wont-match.img"}, + ) + assert entry["filename"] == "default.img" + + +def test_partition_options_defaults_applied() -> None: + _, entry = conf.partition_options([("--name", "x")]) + assert entry["type"] == "00000000-0000-0000-0000-000000000000" + assert entry["bootable"] == "false" + assert entry["readonly"] == "true" + assert entry["sparse"] == "false" + assert entry["filename"] == "" + + +# --------------------------------------------------------------------------- +# parse_disk_line (line-level) +# --------------------------------------------------------------------------- + + +def test_parse_disk_line_full_line() -> None: + line = ( + "--disk --type=ufs --size=137438953472 " + "--sector-size-in-bytes=4096 --write-protect-boundary=0 " + "--grow-last-partition" + ) + result = conf.parse_disk_line(line) + assert result is not None + assert result["type"] == "ufs" + assert result["size"] == "137438953472" + assert result["SECTOR_SIZE_IN_BYTES"] == "4096" + assert result["GROW_LAST_PARTITION_TO_FILL_DISK"] == "true" + + +def test_parse_disk_line_returns_none_when_not_disk_line() -> None: + # A line that doesn't start with --disk is silently dropped. + assert conf.parse_disk_line("--partition --name=x --size=1KB") is None + + +# --------------------------------------------------------------------------- +# parse_partition_lines (line-level) +# --------------------------------------------------------------------------- + + +def test_parse_partition_lines_single_partition() -> None: + lines = [ + "--partition --lun=0 --name=rootfs --size=1KB " + "--type-guid=B921B045-1DF0-41C3-AF44-4C6F280D3FAE", + ] + result = conf.parse_partition_lines(lines) + assert "0" in result + assert len(result["0"]) == 1 + assert result["0"][0]["label"] == "rootfs" + assert result["0"][0]["size_in_kb"] == "1" + + +def test_parse_partition_lines_groups_by_lun() -> None: + lines = [ + "--partition --lun=0 --name=a --size=1KB " + "--type-guid=00000000-0000-0000-0000-000000000001", + "--partition --lun=0 --name=b --size=2KB " + "--type-guid=00000000-0000-0000-0000-000000000002", + "--partition --lun=1 --name=c --size=4KB " + "--type-guid=00000000-0000-0000-0000-000000000003", + ] + result = conf.parse_partition_lines(lines) + assert sorted(result.keys()) == ["0", "1"] + assert [p["label"] for p in result["0"]] == ["a", "b"] + assert [p["label"] for p in result["1"]] == ["c"] + + +def test_parse_partition_lines_skips_non_partition_lines() -> None: + # Lines not starting with --partition are silently ignored by this helper. + lines = ["--disk --type=ufs --size=1024", "--something-else"] + assert conf.parse_partition_lines(lines) == {} + + +# --------------------------------------------------------------------------- +# loader public API: load() + dispatcher +# --------------------------------------------------------------------------- + + +def test_conf_load_full(tmp_path) -> None: + p = tmp_path / "p.conf" + p.write_text( + "# header\n" + "--disk --type=ufs --size=1073741824 --sector-size-in-bytes=4096\n" + "--partition --lun=0 --name=rootfs --size=1KB " + "--type-guid=B921B045-1DF0-41C3-AF44-4C6F280D3FAE --filename=r.img\n" + ) + result = conf.load(str(p)) + assert result["disk"]["type"] == "ufs" + assert result["disk"]["size"] == "1073741824" + assert result["partitions"]["0"][0]["label"] == "rootfs" + assert result["partitions"]["0"][0]["filename"] == "r.img" + + +def test_conf_load_image_map_overrides_filename(tmp_path) -> None: + p = tmp_path / "p.conf" + p.write_text( + "--disk --type=ufs --size=1073741824 --sector-size-in-bytes=4096\n" + "--partition --lun=0 --name=rootfs --size=1KB " + "--type-guid=B921B045-1DF0-41C3-AF44-4C6F280D3FAE --filename=default.img\n" + ) + result = conf.load(str(p), image_map={"rootfs": "override.img"}) + assert result["partitions"]["0"][0]["filename"] == "override.img" + + +def test_conf_load_rejects_two_disk_lines(tmp_path) -> None: + p = tmp_path / "p.conf" + p.write_text("--disk --type=ufs --size=1\n--disk --type=ufs --size=2\n") + with pytest.raises(conf.ConfParseError, match="more than one --disk"): + conf.load(str(p)) + + +def test_conf_load_rejects_missing_disk_line(tmp_path) -> None: + p = tmp_path / "p.conf" + p.write_text( + "--partition --lun=0 --name=x --size=1KB " + "--type-guid=00000000-0000-0000-0000-000000000001\n" + ) + with pytest.raises(conf.ConfParseError, match="no --disk entry"): + conf.load(str(p)) + + +def test_dispatcher_routes_conf_extension(tmp_path) -> None: + p = tmp_path / "p.conf" + p.write_text( + "--disk --type=ufs --size=1073741824 --sector-size-in-bytes=4096\n" + "--partition --lun=0 --name=x --size=1KB " + "--type-guid=00000000-0000-0000-0000-000000000001\n" + ) + result = load(str(p)) + assert result["disk"]["type"] == "ufs" + + +def test_dispatcher_rejects_unknown_extension(tmp_path) -> None: + p = tmp_path / "p.toml" + p.write_text("not actually parsed") + with pytest.raises(UnsupportedFormatError, match="No loader registered"): + load(str(p)) + + +# --------------------------------------------------------------------------- +# main() smoke test (end-to-end via gen_partition.main) +# --------------------------------------------------------------------------- + + +def test_main_produces_xml_for_minimal_conf(tmp_path) -> None: + conf_path = tmp_path / "partitions.conf" + conf_path.write_text( + "# comment line, should be ignored\n" + "\n" + "--disk --type=ufs --size=1073741824 --sector-size-in-bytes=4096\n" + "--partition --lun=0 --name=rootfs --size=1KB " + "--type-guid=B921B045-1DF0-41C3-AF44-4C6F280D3FAE --filename=r.img\n" + ) + out = tmp_path / "partitions.xml" + + rc = gp.main(["gen_partition", "-i", str(conf_path), "-o", str(out)]) + + assert rc == 0 + content = out.read_text() + assert "physical_partition" in content + assert 'label="rootfs"' in content + assert 'size_in_kb="1"' in content + + +def test_main_image_map_override_applied(tmp_path) -> None: + conf_path = tmp_path / "partitions.conf" + conf_path.write_text( + "--disk --type=ufs --size=1073741824 --sector-size-in-bytes=4096\n" + "--partition --lun=0 --name=rootfs --size=1KB " + "--type-guid=B921B045-1DF0-41C3-AF44-4C6F280D3FAE --filename=default.img\n" + ) + out = tmp_path / "partitions.xml" + + rc = gp.main( + [ + "gen_partition", + "-i", str(conf_path), + "-o", str(out), + "-m", "rootfs=override.img", + ] + ) + + assert rc == 0 + assert 'filename="override.img"' in out.read_text() + + +def test_main_rejects_two_disk_lines(tmp_path, capsys) -> None: + conf_path = tmp_path / "partitions.conf" + conf_path.write_text( + "--disk --type=ufs --size=1\n" + "--disk --type=ufs --size=2\n" + ) + out = tmp_path / "partitions.xml" + + rc = gp.main(["gen_partition", "-i", str(conf_path), "-o", str(out)]) + + assert rc == 1 + assert "more than one --disk" in capsys.readouterr().out