With the target exporting CPU_STRIDE/NUM_CPUS offsets (see the companion
kernel commit), teach the host coredump scripts to enumerate one current
thread per CPU.
elf_parser.py exposes the two new offset-table entries and, instead of
capping get_kernel_thread_info_offset() at a hardcoded ceiling, bounds
it by the count actually read from the target binary so an older build
with fewer entries degrades a newer field to "not present" rather than
reading out of bounds.
gdbstub.py's qfThreadInfo walk now enumerates one current thread per CPU
(cpus[i].current = K_CURR_THREAD + i * CPU_STRIDE) instead of one total.
Targets whose offsets table lacks CPU_STRIDE/NUM_CPUS (older or non-SMP
builds) get None back for those entries and degrade to the original
single-CPU behavior.
Signed-off-by: Appana Durga Kedareswara rao <appana.durga.kedareswara.rao@amd.com>
Previously, the last --warn-paths argument would replace any previous one
that was specified on the command line. Use action="append" to ensure that
all of the arguments are collected.
Signed-off-by: Chris Friedt <chris@fr4.co>
Board extensions do not require the presence of a "Kconfig.<board_name>"
file in order to function correctly, so we should use "osource" when
sourcing these board Kconfig files. The primary board directory's
Kconfig file is still sourced normally, as this Kconfig file is
required.
Signed-off-by: Daniel DeGrasse <daniel.degrasse@analog.com>
The QEMU monitor thread's _thread_update_instance_info() stamped the
instance reason with "Unknown Error" whenever the harness provided no
reason, regardless of the resulting status. Passing harnesses supply
no reason, so every passing QEMU test instance silently carried
reason="Unknown Error". The classic console output only prints the
reason for failures, which kept this invisible, but trying to view
status using other means reveals this issue.
Only fall back to "Unknown Error" when the status is actually a
failure (failed/error) and no more specific reason is known. For
non-failure results clear the reason instead of keeping it, so a
stale reason from an earlier retry iteration cannot stick to a
now-passing instance.
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
GdbStub_ARM64 never implemented arch_supports_thread_operations(), so
it always inherited the base class's `return False`. In practice this
meant `info threads` in GDB only ever showed the single faulting
thread, even for CONFIG_DEBUG_COREDUMP_MEMORY_DUMP_THREADS dumps that
structurally contain every thread's k_thread struct and stack -- the
data was already there, this script just had no path to expose it.
Add that path by reading each non-current thread's saved context out
of its k_thread.callee_saved struct. On ARM64,
z_arm64_context_switch() (arch/arm64/core/switch.S) stores a
non-running thread's x19-x29, sp_elx, and lr directly into that struct
(unlike Cortex-M, which pushes a hardware exception frame onto the
thread's own stack), and a resumed thread continues via `ret` using
the saved lr, so lr doubles as the thread's resume PC.
The generic thread-info offsets table (subsys/debug/thread_info.c)
only exposes sp_elx individually, as
THREAD_INFO_OFFSET_T_STACK_PTR = offsetof(k_thread, callee_saved.sp_elx).
Derive the base of the callee_saved struct from that single offset
instead of adding new table entries, keeping this a self-contained
script change.
Enabling thread operations makes the base-class qThreadExtraInfo path
reachable, which had an off-by-one: thread_id is 1-based and indexed as
thread_id - 1, but the guard used `len(thread_ptrs) > thread_id`, so the
last enumerated thread got an empty response and lost its name/state in
`info threads`. Use `>=` so the final thread is included.
Validated on versalnet_apu_se9 hardware coredumps: non-SMP THREADS-mode
scenarios now enumerate all threads (e.g. 10 for the coredump_threads
ztest: 7 worker threads, test_crash, idle, main) with correct names
and distinct, semantically valid backtraces, with no change to
MIN/LINKER_RAM or SMP scenarios (which have no thread-list data to
walk in the first place).
Signed-off-by: Appana Durga Kedareswara rao <appana.durga.kedareswara.rao@amd.com>
During flashing, a gdb load command is performed.
This command prints out each segment it writes and ask the user to press
enter or 'c' to continue when it prints more lines that the height of the
terminal. When performing "west flash" in a small terminal, this requires
user interaction during flashing which is both annoying and unnecessary.
This behavior is disabled using "set pagination off".
Signed-off-by: Paul Wedeck <paulwedeck@gmail.com>
The required-with-a-default check runs on the property settings
left after includes are merged, so a binding that inherits a
default and deliberately overrides it with required was reported
even though nothing is wrong.
That combination is well defined: the required check runs first
and errors out when the property is absent, so the inherited
default never applies. Keep a copy of the properties declared
locally and warn only when both settings come from the same file.
Included files are merged into the raw contents in place, which
also covers any nested child binding, so the copy is taken before
the merge and handed down to each child binding in turn. A child
binding which is only inherited declares nothing of its own and
so gets an empty copy. Add tests covering the reported and the
exempt cases, up to the grandchild binding level.
Assisted-by: Claude Code:opus-4-8
Signed-off-by: Sylvio Alves <sylvio.alves@espressif.com>
Add pytest unit tests for the CMake and Kconfig style checkers: one
flagged and one clean snippet per rule, asserting the exact set of rules
raised. Snippets are passed as strings, so no fixture files are committed
(which the compliance checks would otherwise scan). Both suites run in
the Scripts tests workflow.
Assisted-by: Claude Code:claude-opus-4.8
Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
The if-blank and decl-blank checks assumed single-line statements, so a
backslash-continued 'if' condition produced false positives: the blank
required after 'if' was checked at the keyword line (a continuation
line), and the block-opener exemption for declarations tested that
continuation line instead of the opener keyword.
Follow continuations to the statement's start and end in both checks.
Also require the continuation backslash to be the final character, since
a backslash before trailing whitespace escapes it rather than continuing
the line.
Assisted-by: Claude Code:claude-opus-4.8
Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
Add a workflow that runs the test suite on native_sim under the
sanitizers and reports what they find to the GitHub code scanning
dashboard, as the dynamic counterpart to the CodeQL and GCC
analyzer scans. Those infer what might go wrong; this records what
actually did, so the findings are proven reachable and uploaded at
error level. Coverage is bounded by what the tests exercise, which
is the opposite trade-off, and neither kind of scan subsumes the
other.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
cmake/sca/gcc has been able to drive the GCC static analyzer for
some time, but nothing in CI uses it. Wire it up and report the
findings to the GitHub code scanning dashboard. Unlike the CodeQL
and Eclair scans this needs no license and no extra tooling:
-fanalyzer ships with the toolchain the tree is already built
with.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
The script only ever adds to a pull request: labels, review requests and
an assignee accumulate across runs. There is no way to correct one that
was staffed badly, or to re-apply the current MAINTAINERS.yml to a pull
request that was opened before an area changed hands.
Add --reset, which works out the new staffing first and then, just
before applying it, undoes what an earlier run decided: it withdraws the
review requests, removes every assignee and strips the area labels that
MAINTAINERS.yml defines, leaving the pull request labelled, reviewed and
assigned as if it had just been opened.
Doing this late rather than on entry matters twice over. Every skip
condition -- draft, closed, more than MAX_FILES changed files -- has had
its say by then, so a reset cannot strip a pull request the run goes on
to decline. And the reviewers about to be requested again are known, so
their existing request is left in place instead of being withdrawn and
re-sent, which would notify them twice.
Four things survive the reset:
- Reviewers who submitted a review or commented on the pull request or
one of its review threads. They are involved by their own action,
and dropping their request would either lose that or notify them
again. This covers approvals and changes-requested reviews
regardless of the verdict.
- Team review requests. The script only ever requests individuals, so
a team was requested by hand and a reset has no basis to withdraw it.
- Size labels, which update_size_labels recomputes and prunes on every
run anyway; removing them here would double-remove.
- Labels the script does not own, such as 'bug' or backport labels.
They are not derived from MAINTAINERS.yml, so a reset has no basis
for second-guessing whoever added them.
People who removed themselves from the review request are not re-added:
_add_reviewers already refuses to request them, and the reset's own
removals are made by the token account, so they do not read as
self-removals either.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
A pull request matching more than MAX_LABELS (10) area labels had its
label step skipped entirely, on the grounds that labelling a change with
a dozen areas says nothing. The result was worse: the broadest pull
requests, the ones hardest to route, ended up with no area label at all.
Rank the collected labels by how much of the pull request the areas
carrying them cover, and apply the MAX_LABELS highest-ranked ones
instead, so the same limit now caps how many labels are applied rather
than deciding whether any are. A label's rank is the sum of the file
weights of every touched area carrying it, falling back to the plain
file count for areas weighted 0. Those weights are zeroed for assignee
selection -- meta-areas, files that only matched CMakeLists.txt,
repeated matches of one Platform area -- but such an area still
describes what the pull request changes, and without the fallback a
documentation-only pull request would rank 'area: Documentation' last
and drop it first. Ties break by name to keep the outcome
deterministic.
Size labels take no part in this. They are computed for the pull
request alone and update_size_labels has already pruned the stale ones,
so they neither count towards the limit -- ten area labels plus a size
label is not an overflow -- nor can be dropped by it.
The limit governs what a run applies; no label is ever taken off the
pull request, whether a human or an earlier run put it there. --reset
is the way to clear those.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Changed files that MAINTAINERS.yml cannot staff (files in under-covered
areas, files in no area at all) fall back to two heuristics: GitHub's own
suggestedReviewers, then the recent contributors to those files. Both
ran whenever such a file was present, however well staffed the rest of
the pull request already was, so a single orphaned file was enough to
append lower-confidence names to a review request that already held the
people responsible for the change.
Skip both heuristics once the touched areas have yielded
MIN_TOTAL_REVIEWERS (3) candidates other than the author, who cannot
review their own pull request. The count is taken across all areas, so
two thinly staffed areas that together cover the pull request no longer
trigger the fallback either.
Also limit the commit walk to the last HISTORY_MAX_AGE_DAYS (365) days.
A contributor who has not touched a file within a year is unlikely to
still be the right person to ask, and on long-lived files the unbounded
history filled every heuristic slot with people who have moved on. The
window is applied by GitHub via the 'since' parameter, so it also cuts
the API cost of the walk.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
An area that names no maintainers, or at most two collaborators, is
considered under-covered: it cannot staff a review on its own, so recent
Git contributors to its files are added as heuristic reviewers.
That test misfires on meta-areas. They match large parts of the tree by
design and name few people on purpose, so a documentation- or
samples-only pull request would walk the history of every file it
touches and request reviews from people on top of the maintainers and
collaborators who already own the area.
Skip areas carrying 'meta: true' when collecting under-covered areas.
Files matched only by a meta-area no longer feed the heuristic; files
that a thin non-meta area also claims still do, as do orphaned files
that match no area at all.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Meta-areas, i.e. areas that cut across the whole tree instead of owning
one subsystem, were hardcoded as name lists in two CI scripts:
set_assignees.py (META_AREAS) and sync_maintainer_tests.py
(_META_AREAS). The two lists had drifted apart and both had gone stale:
they named "Release Notes" and "Boards", neither of which is an area in
MAINTAINERS.yml anymore.
Move the classification into MAINTAINERS.yml itself as an optional
per-area 'meta: true' key, so it lives in one place next to the area it
describes and new meta-areas can be added without touching any script.
get_maintainer.py validates the key as a boolean, exposes it as
Area.meta and prints it alongside the other area fields.
Both scripts now read that key instead of their own list. As the two
lists are unified, membership changes slightly: Benchmarks is no longer
weighted when picking a pull request assignee, and Documentation and
Release no longer get auto-generated 'tests' entries.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Add a `--gdb-pre-debug` option to allow appending commands to run before
starting a debugging session, after the gdb `load` command is issued.
This can be used in targets that require a specific setup for the
`debug` command, such as initialization commands to leave the board
ready and in a known state.
Signed-off-by: Ricardo Cañuelo Navarro <rcn@igalia.com>
Both test plan scripts imported TwisterStatus through the package path
'pylib.twister.twisterlib.statuses'. Modules inside twisterlib import
each other with absolute 'twisterlib.<module>' names, which only resolve
when scripts/pylib/twister is on sys.path - something only the
scripts/twister launcher did. That spelling worked by accident as long
as statuses.py had no intra-package imports; commit fc599d0e3e added
'from twisterlib.error import StatusAttributeError' to it and the import
started raising ModuleNotFoundError.
test_plan.py aborted outright. In test_plan_v2.py the same import sat
inside a bare try/except ImportError that returned 0, so _count_errors()
always reported no errors and the script exited 0 even when test
configurations failed.
Put scripts/pylib/twister on sys.path in both scripts and import
'twisterlib.statuses' directly, matching every other consumer of the
package, then drop the try/except so an unimportable TwisterStatus is a
hard failure rather than a silently miscounted result. Importing via
both spellings would otherwise create two distinct TwisterStatus enum
classes whose members compare unequal under 'is'.
Fixes#114208
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Use the ZEPHYR_BASE constant from zephyr_ext_common instead of relying
on the ZEPHYR_BASE environment variable, which may not be set.
Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
BinaryHandler captures the test binary's stderr to its own file,
handler_stderr.log, which no report ever reads. Whatever the
binary wrote there is therefore absent from twister.json and from
everything derived from it.
This is most visible with UndefinedBehaviorSanitizer on
native_sim. It writes its diagnostics to stderr rather than
stdout, so a run it aborts is recorded as a bare "rc=1": the
report says the test failed and nothing at all about why, even
though the reason was captured and sits on disk next to the
build. The same applies to any crash or loader failure that
reports on stderr.
Append the captured stderr to the log already selected for a
failing instance, rather than replacing it, so the stdout
transcript that shows how far the test got is preserved and the
stderr diagnostic explaining the failure follows it. Only failing
instances are affected, and only when stderr is non-empty, so
passing runs and reports for handlers that do not capture stderr
are unchanged.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Raise the minimum required CMake version from 3.20.0 (documented as
3.20.5) to 3.28.0, which is satisfied by the CMake 3.28.3 package
shipped in the Ubuntu 24.04 LTS repositories. Ubuntu 24.04 is the
current Ubuntu LTS release targeted by the Zephyr getting started
guide, and by the time of the next Zephyr release, Ubuntu 22.04 will be
within months of its end of standard support. Users of distributions
shipping an older CMake can use the Kitware APT repository or a
pip-installed CMake, as the documentation already suggests.
Raising the floor to 3.28 unlocks a range of modern CMake features for
the build system, among which the cmake_file_api() command (3.27),
file(COPY_FILE) (3.21), block()/endblock() (3.25), and allows removal
of several version-conditional workarounds.
The tree-wide cmake_minimum_required() occurrences in samples, tests
and boards are updated accordingly, together with the documentation
and the sysbuild CMake presets. The IAR C-STAT integration keeps its
own higher requirement (4.1.0).
Assisted-by: Claude:fable-5
Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
A module appears twice in the SBOM: as a "-sources" package (its
checked-out source tree, in the zephyr document) and as a "-deps"
package (its upstream identity for vulnerability tracking, in the
modules-deps document). Nothing connected the two, so a consumer could
not tell they describe the same module.
Add a VARIANT_OF relationship from each source package to its
dependency package (rendered as hasVariant in SPDX 3.0), so the checked-
out sources are tied to the upstream dependency they are a variant of.
The same link is added between zephyr-sources and zephyr-deps.
Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
Assisted-by: Cursor:opus-4.8
ExternalDocumentRef was emitted after Created, past the Document
Creation Information section. Strict tag-value parsers reject that
("unknown tag ExternalDocumentRef in CreationInfo section") and fail to
load the document. Emit it right after DocumentNamespace and before
Creator, where the specification places it.
Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
Assisted-by: Cursor:opus-4.8
The role of a module's "-sources" package versus its "-deps" package is
not self-evident, and a "-deps" package is emitted for every module in
the manifest even when its code is not built, which looks surprising.
Attach a package comment to each explaining what it represents and why
it is present.
This also replaces the "Utility target; no files" comment that was
applied to every fileless package: it was left over from the CMake
utility targets (now excluded) and mislabelled the source and
dependency packages.
Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
Assisted-by: Cursor:opus-4.8
CMake UTILITY targets (menuconfig, ram_report, run/flash/debug, code
generation helpers, ...) are build-system conveniences, not software
components. They produced empty, purposeless packages that only added
noise to the build SBOM, so skip them while walking the codemodel.
Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
Assisted-by: Cursor:opus-4.8
Components lacking a supplier or a package URL are flagged by SBOM
quality tooling and cannot be matched against vulnerability databases.
Populate both for Zephyr and its modules from the metadata already in
zephyr.meta: the git remote yields a supplier organization and a
revision-pinned purl. Modules recorded under "url" instead of "remote"
are now handled too, and curated purls from a module's security
metadata are left untouched.
In SPDX 3.0 the supplier is an Agent reference rather than a string, so
each package's suppliedBy points at a shared, deduplicated Organization
element.
Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
Assisted-by: Cursor:opus-4.8
An SBOM should declare who authored it and which tool version produced
it; both were missing, so provenance/quality checks scored zero. Add
"The Zephyr Project" as the creating organization and stamp the
generator with the Zephyr version read from the VERSION file.
The values are recorded on the SBOM graph metadata by the walker and
emitted by both serializers: as SPDX 2.x Creator lines, and in SPDX 3.0
as an Organization in CreationInfo.createdBy plus a packageUrl on the
tool (which has no dedicated version field).
Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
Assisted-by: Cursor:opus-4.8
The compliance checks currently run strictly one after the other. Most of
the total runtime is spent in the Kconfig-based checks, each of which
parses a full Kconfig tree on its own.
Add a -p/--parallel [N] option that runs the checks in a pool of N worker
processes (one per CPU by default). Each check runs in its own process,
which also isolates the environment variable mutations and module imports
the Kconfig checks perform. The Kconfig-based checks are scheduled first
so the slowest checks are not left running alone at the end.
Enable the new option in the compliance workflow, where the runtime of
the Kconfig checks now roughly amounts to the single slowest check
instead of the sum of all of them.
Assisted-by: Claude:fable-5
Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
Each compliance check runs inside a try/except that catches BaseException
and turns any error into a failure for that check. This also caught
KeyboardInterrupt, so pressing Ctrl-C only aborted the check that happened
to be running and the loop carried on with the next one instead of
stopping the whole run.
Re-raise KeyboardInterrupt from the per-check handler so it propagates, and
handle it in main() with a clean "Interrupted" exit instead of dumping a
traceback.
Assisted-by: Claude:opus-4.8
Signed-off-by: Benjamin Cabé <benjamin@zephyrproject.org>
Depending on conditions, PermissionError may also be raised if there
is no executable to run.
So let's also handle it in the same way.
This expands on the fix from a18fd195bf
Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
checkpatch reported a false-positive SPACING error for pointer
declarations whose type name comes from a function-like macro, e.g.
"struct GPIO_FAST_DISPATCH_TYPE(NODE) *spec", because the macro's
parentheses were parsed as a call and the '*' as a multiply. Add a
branch to annotate_values() that consumes a struct/union/enum tag plus a
function-like macro as part of the type so the '*' is seen as a pointer.
Requiring the tag avoids misclassifying genuine multiplies like
"u32(5) * 2". Closes#113747.
Assisted-by: Claude:claude-opus-4.8
Signed-off-by: Perry Naseck <pnaseck@media.mit.edu>
Assisted-by tag was introduced in doc/contibute/guidelines.rst
for attributing AI tool contributions.
checkpatch.pl doesn't recognise this tag and throws BAD_SIGN_OFF
issues for missing email address.
This fix imports linux kernel support for Assisted-by tag to
zephyr (d1db411).
It adds Assisted-by to the recognised $signature_tags list,
skips email validation since Assisted-by uses the
[Agent Name]:[Model Version] format and throws a warning
if the value doesn't match the expected format.
Signed-off-by: Ioannis Damigos <ioannis.damigos.uj@renesas.com>
add the delaring macros from
zephyr/sys/device_mmio.h
as to list so it does not complain about it.
Signed-off-by: Fin Maaß <f.maass@vogl-electronic.com>
Several small copy-paste items in testplan.py:
- The `set(p.platform.name for p in self.instances.values())` expression
used to refresh selected_platforms appeared four times (three in load()
and once in apply_filters()); extract _refresh_selected_platforms().
- The five-argument instance.create_overlay(...) call with the same asan/
ubsan/coverage options appeared in load_from_file() and apply_filters();
extract _create_overlay(instance). The loop variable `platform` at the
first site equals instance.platform (the instance is built for it), so
reading instance.platform is equivalent.
- The three YAML schema paths all rebuilt the ZEPHYR_BASE/scripts/schemas/
twister prefix; hoist it to a TWISTER_SCHEMA_DIR constant.
- The "Not runnable on device" reason was set in one method and string-
compared in another 400 lines away; promote it to the module constant
NOT_RUNNABLE_ON_DEVICE_REASON so the two cannot drift.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
zephyr_module.parse_modules(ZEPHYR_BASE) walks the whole workspace for
module manifests, and twister ran it twice per invocation: once in
add_parse_arguments() to build the --board-root default and again in
TwisterEnv.__init__() to collect the snippet/soc/dts/arch roots. Wrap it
in a functools.cache helper (_parse_modules) so the scan happens once; the
result is read-only at both sites, so sharing it is safe.
While there, replace the five near-identical "settings.get('<x>_root')
-> append to self.<x>_roots" blocks in TwisterEnv.__init__ with a single
loop over a {setting_key: roots_list} mapping. project / snippet_root and
project / Path(soc_root) are equivalent, so wrapping every value in Path()
is a no-op.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
process() was a 287-line if/elif chain over nine pipeline ops, and every
branch repeated the same ~5-line "except StatusAttributeError" handler
(force ERROR, set the 'Incorrect status assignment' reason, block the
remaining cases) followed by a finally that enqueues the next op.
Split each op into its own _op_<name> method and drive them from a
_PIPELINE_OPS name->method table, so process() is now a short dispatcher.
The repeated exception body becomes a single _mark_status_error() helper
(the coverage op keeps its "on <op>" reason variant via op_label). Each
handler keeps its own control flow, including the irregular cases: 'run'
still re-queues the message on NoDeviceAvailable/RequiredAppNotReady,
'report' still clears next_op on error, and 'cleanup' still neither
enqueues nor reports.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
XUnitXMLReport.create() and create_with_all_testsuites() shared ~70% of
their bodies: loading the JSON, building the <testsuite> element with its
eight zeroed attributes and version property, the full-report testcase
loop, the counter-finalisation block, and the indent/tostring/write tail.
Factor these into _read_json(), _new_testsuite(), _finalize_testsuite(),
_emit_full_report_testcases() and _write_report(), and rewrite both public
methods in terms of them. create() keeps its per-platform grouping and its
full_report vs per-suite-summary branch; create_with_all_testsuites() keeps
its per-suite grouping and extra platform/architecture properties.
No behaviour change: rendering both methods over a synthetic report and
comparing canonicalised XML (ET.canonicalize) is byte-identical to the
previous code across full/summary/target/all-testsuites variants and both
detailed_skipped_report settings. The only difference is that the
all-testsuites <testsuite> now emits its time/timestamp attributes in the
same order as create() - JUnit attribute order is not significant and no
test pins it.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
QEMUHandler (POSIX) and QEMUWinHandler (Windows) duplicated _get_cpu_time,
_create_command, get_fifo, the pid-file half of _set_qemu_filenames, and a
byte-identical instance-info updater that had drifted into two names
(_thread_update_instance_info vs _monitor_update_instance_info). The
post-status timeout-extension block, with its bare 30/2 second literals,
was copied into both monitor loops.
Introduce a QEMUHandlerBase(Handler) holding these shared pieces and have
both handlers inherit it. The magic timeout values become the
COVERAGE_TIMEOUT_EXTENSION / POST_STATUS_TIMEOUT_EXTENSION constants,
consumed through a single _extend_timeout_on_status() helper. The Windows
handler drops its now-identical _set_qemu_filenames entirely; the POSIX
one keeps a two-line override that also sets the fifo path.
The two monitor loops themselves are deliberately left in the subclasses:
their wait/read mechanisms differ fundamentally (select.poll on POSIX
fifos vs a reader-thread Queue on Windows) and the Windows path is not
exercised by Linux CI, so merging them would risk an untestable
regression. The public method names the tests patch (_get_cpu_time,
_thread_update_instance_info, _thread_get_fifo_names, _set_qemu_filenames)
are preserved, so behaviour is unchanged.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Pytest and Ctest each duplicated an almost identical run() skeleton and a
byte-identical _update_test_status(); the two differed only in the tool
name used in reason strings, the exception type run() catches, and
whether pytest's PYTHONPATH env dependencies are injected.
Introduce a ReportHarness(Script) base that owns run() and
_update_test_status(). Subclasses now declare a report_tool label and a
run_exception type, and override the _run_report_command() hook when they
need to alter how the tool is invoked (Pytest injects env dependencies).
The triple of ExitCodes meaning "the tool errored" becomes the module
constant REPORT_ERROR_EXIT_CODES. Script keeps its own script-oriented
run()/_update_test_status(); Bsim is unaffected.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
A formal review request is not always possible. GitHub rejects one for
anybody without collaborator status, so an area maintainer or collaborator
listed in MAINTAINERS.yml who lacks push access was silently dropped, and
the request as a whole may be refused for reasons the script cannot
anticipate. In both cases the people responsible for the changed code
never learned the PR existed.
Fall back to asking them by name. Users who cannot be added to the review
request are @mentioned in a comment requesting their review, which
notifies them regardless of their permissions. This covers both the
non-collaborator case and whatever the review request could not
accommodate, including the tail dropped when a rejected bulk request is
retried with fewer candidates.
The comment is maintained in place rather than appended: it carries a
hidden MENTION_MARKER, so a re-run finds its own previous comment and
edits it only when the set of people actually changed. Re-running over a
PR therefore neither duplicates the comment nor adds timeline noise.
Users who removed themselves from the review request are excluded, and
the self-removal test now runs before the collaborator test so that an
opt-out is never routed to the mention path instead. Deleted accounts are
still skipped outright, there being nobody to mention. A failure to post
the comment is logged and swallowed, like the review request itself, so
that a repository without comment write access still completes its run.
Assisted-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
MAX_REVIEWERS (15) drove two behaviours: the candidate list was truncated
to the remaining vacancy, and a PR already holding that many reviewers
discarded its candidate list entirely in favour of the primary area's
maintainers. Both predate the tiered candidate ordering, and neither
still earns its keep.
The cap was never GitHub's. GitHub documents no limit on how many
reviewers a pull request may carry, and PRs with more than 15 individual
reviewers requested (no teams involved) are observable in this very
repository, so the number was self-imposed. Its only effect was to drop
people who were legitimately responsible for part of the change.
The overflow strategy was worse than redundant. It compared
MAX_REVIEWERS against a set that unions submitted reviews with pending
review requests, so a well-reviewed PR could take the restricted path
with no pending requests at all and plenty of room to add more. Now that
maintainers are ordered ahead of collaborators, the ordering already does
what the strategy was reaching for.
Drop the cap and request everyone eligible. This also retires
_add_reviewers()'s primary_maintainers and extra_reviewers parameters,
which were referenced only from the removed branch; additional_reviews is
folded into the candidate list by _build_reviewer_candidates and needs no
separate path. Should GitHub refuse the full set anyway, the call is
retried once with the first REVIEWER_RETRY_BATCH candidates, which the
tier ordering makes the maintainers, so a rejection cannot leave the PR
with no reviewers at all.
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Assisted-by: Claude Opus 4.8 <noreply@anthropic.com>
Every reviewer tier is derived from the areas a changed file maps to, so a
file that matches no area in MAINTAINERS.yml contributes nobody. A PR
made up only of such files ends up with an empty candidate list, and the
review request is skipped entirely: nobody is asked to look at it.
Feed unmatched files into the same heuristics already used for
under-covered areas.
The heuristic pass now triggers on the union of under-covered-area files
and orphaned files, so a mixed PR still walks only the files that are
actually unstaffed, and a fully covered PR still issues no extra query.
Heuristic picks stay appended after the maintainer and collaborator
tiers, so they only ever fill leftover review slots.
Assisted-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Some areas in MAINTAINERS.yml name no maintainers, or only a handful of
collaborators. When a PR touches such an area, the reviewer candidate
list built from MAINTAINERS.yml alone is too thin to attract a meaningful
review, and nobody responsible for the change is requested.
Supplement these areas heuristically. An area is treated as
under-covered when it names no maintainers, or at most
THIN_AREA_COLLABORATORS (2) collaborators. For every such area two
sources are tried in order, and the result is appended after the
maintainer and collaborator tiers so that it only ever fills leftover
review slots.
First, GitHub's own suggestions for the pull request
(PullRequest.suggestedReviewers), which GitHub derives from both commit
history and past review comments. That is the better-informed signal and
costs a single query instead of one per changed file, so suggestions
GitHub flagged as authors of the changed code are ranked ahead of
comment-only ones. The field is only available over GraphQL, so this
issues a raw query through PyGithub's requester, which reuses the
existing token and session; no new dependency is needed. Every failure
path (transport error, GraphQL errors, unexpected shape, null list)
degrades to an empty list, so a bad response can only cost the
suggestion, never the run.
Second, when GitHub suggests nobody, the recent contributors to the
changed files the under-covered areas own, read from the commit history.
This fallback is not redundant: sampling a dozen open PRs showed GitHub
returning suggestions for only about a third of them, with no visible
correlation to how many reviewers were already requested. The commit
walk also stays scoped to the files that are actually short of reviewers,
whereas GitHub's suggestions cover the PR as a whole. History is read
through the GitHub API rather than a local 'git log'/'git blame': the API
resolves commits to real GitHub logins (author emails cannot be mapped to
logins reliably) and does not depend on the CI checkout having full
history.
In both cases the PR author and everyone already on the candidate list
are excluded, so these last-resort slots are not spent on reviewers the
PR already has, and the commit walk is bounded by
HISTORY_COMMITS_PER_FILE, MAX_HISTORY_FILES, and MAX_HISTORY_REVIEWERS to
keep the API cost predictable.
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
Assisted-by: Claude Opus 4.8 <noreply@anthropic.com>
The reviewer candidate list was built by iterating touched areas in
descending weight order and appending each area's maintainers immediately
followed by its collaborators. _add_reviewers() then truncates the
filtered list to the remaining reviewer vacancy (MAX_REVIEWERS minus the
existing reviewer count).
On a fresh PR that touches many areas, the collaborators of the
highest-weight areas were reached before the maintainers of the
lower-weight areas, so the review-request slots could be filled with
collaborators while the maintainers of several touched areas were never
requested.
Build the candidate list in two tiers instead: every touched area's
maintainers first (in descending weight order), plus the manifest /
MAINTAINERS.yml-change and deferred-file-group maintainers, followed by
all area and path-specific collaborators. Truncation to the vacancy now
drops collaborators before it drops any maintainer. Set-derived inputs
are sorted so candidate ordering is deterministic.
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
TwisterExecutor.execute() ran twister to enumerate the tests matching a
strategy's patterns and, on any failure, logged a warning and returned
an empty list. A crash - a missing Python dependency, an import error,
an OOM kill, a test config that fails schema validation - was therefore
indistinguishable from "this change needs no tests": the plan came back
empty, .testplan reported zero nodes, and the workflow skipped the whole
twister-build matrix. A change could merge with no coverage at all
because the selector broke, not because it was safe.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Anas Nashif <anas.nashif@intel.com>
