fix(ci): report malformed CTK redistrib metadata instead of tracebacking - #2550
Open
LeSingh1 wants to merge 1 commit into
Open
fix(ci): report malformed CTK redistrib metadata instead of tracebacking#2550LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
fetch_ctk_redistrib.main() wraps everything in a deliberate handler:
except (ValueError, KeyError, OSError, urllib.error.URLError,
json.JSONDecodeError) as exc:
print(f"ERROR: {exc}", file=sys.stderr)
return 1
Several malformed-manifest shapes escape it because the guards check for
absence, not type.
1. Real redistrib_*.json files carry string-valued top-level keys --
"release_date", "release_label", "release_product" -- next to the
component objects. `metadata.get(component)` returns a str for those, and
`component_info is None` does not reject it, so `component_info.get(...)`
raises `AttributeError: 'str' object has no attribute 'get'`.
2. The manifest is downloaded with `curl -LSs` and no `--fail`
(.github/actions/fetch_ctk/action.yml), so an error page or redirect body
is written to the file. If that body is valid JSON but not an object, the
failure surfaces frames later as `TypeError: argument of type 'NoneType'
is not iterable` or `AttributeError: 'list' object has no attribute 'get'`.
3. A subdir entry that is a bare string rather than an object raises the same
AttributeError from a different line.
Reproduced via `main(argv)` with `--metadata-path`, so no network:
--component release_label -> AttributeError: 'str' object has no attribute 'get'
metadata is null -> TypeError: argument of type 'NoneType' is not iterable
metadata is a JSON array -> AttributeError: 'list' object has no attribute 'get'
subdir entry is a string -> AttributeError: 'str' object has no attribute 'get'
An absent component, by contrast, already returns 1 with a clear message.
Validate the manifest is a JSON object where it is loaded, and check the
component and subdir entries are objects before reaching into them. Also
replace `ctk_subdir in metadata.get(resolved_component, {})` in
filter_components with an explicit dict check: on a string value that `in`
silently becomes a substring test rather than a key lookup.
Adds the first tests for this tool.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
fetch_ctk_redistrib.main()wraps its whole body in a deliberate handler so the tool reports problems rather than crashing:Several malformed-manifest shapes escape it, because the guards test for absence rather than type.
1. String-valued top-level keys are treated as components. Every real
redistrib_*.jsoncarries"release_date","release_label","release_product"at the top level, next to the component objects.metadata.get(component)returns astrfor those, and the guard only rejectsNone:2. A manifest that is valid JSON but not an object.
.github/actions/fetch_ctk/action.yml:77downloads it withcurl -LSs "$CTK_JSON_URL" -o "$CTK_JSON_FILE"— no--fail— so an HTTP error page or redirect body is written to the file. If it parses asnull, an array, or a string, the failure surfaces several frames later.3. A subdir entry that is a bare string rather than an object raises the same
AttributeErrorfrom a different line.Reproduced through
main(argv)with--metadata-path(no network), against a manifest shaped like the real thing:main--component cuda_nvcc(valid)rc=0(correct)--component not_a_componentrc=1,ERROR: "unknown CTK component..."(correct)--component release_labelAttributeError: 'str' object has no attribute 'get'nullTypeError: argument of type 'NoneType' is not iterableAttributeError: 'list' object has no attribute 'get'AttributeError: 'str' object has no attribute 'get'The contrast with the absent-component row is the point: the tool already knows how to report this class of problem, and these paths bypass it.
Fix
filter_components, replacectk_subdir in metadata.get(resolved_component, {})with an explicitisinstance(..., dict)check. On a string value thatinsilently degrades from a key lookup to a substring test — it happens to returnFalsefor the real keys today, but only by luck.Valid manifests are unaffected: the component lookup, the subdir lookup, and the
relative_pathextraction all behave exactly as before.Tests
This tool had no tests. Added
ci/tools/tests/test_fetch_ctk_redistrib.py(which is already run by the nightly tooling job), drivingmain(argv)with--metadata-pathso nothing touches the network:relative_path;rc=1;unknown CTK componentmessage;null/ array / string manifests are reported for both subcommands;filter-componentsskips a string-valued top-level key with the existing "Skipping unsupported CTK component" notice andrc=0.Verification
Executed in full (pure Python, stdlib only, no GPU and no network):
ruff check/ruff format --checkclean on both files;toolshed/check_spdx.pyclean on the new file. Restore done withcpaside +git show upstream/main:<path> >; index verified clean before committing.