iree-org / iree

A retargetable MLIR-based machine learning compiler and runtime toolkit.
http://iree.dev/
Apache License 2.0
2.85k stars 620 forks source link

configure_ci.py interprets `Notes:` as "trailer lines" #19240

Closed bjacob closed 23 hours ago

bjacob commented 1 day ago

All CI jobs on https://github.com/iree-org/iree/pull/19194 were failing since I wrote a nice long description. I made another PR to debug that, https://github.com/iree-org/iree/pull/19239, and what I found is that having a PR description where one line is just

Notes:

gets interpreted by configure_ci.py as trailer_lines, which it then tries to split as key:value and that complains that there is only one term here, not two.

See the CI log here; that PR adds a print(trailer_lines) so we see that it starts with "Notes:": https://github.com/iree-org/iree/actions/runs/11945467240/job/33298195480#step:4:94

Notice the difference between the PR descriptions of https://github.com/iree-org/iree/pull/19239 (failing) and https://github.com/iree-org/iree/pull/19194 (working): in the latter, I removed the : after Notes .

ScottTodd commented 1 day ago

Ah. The script is parsing git trailers (https://git-scm.com/docs/git-interpret-trailers) which are in the key: value format (as produced by git tools). Should add some error handling if the parsing does not match what the script expects, since the input is a PR description that is likely authored by humans instead of tools: https://github.com/iree-org/iree/blob/e1ce3fa407e952ea7cca021fe2d441bbfdcd04c0/build_tools/github_actions/configure_ci.py#L263-L274

ScottTodd commented 1 day ago

Okay, I read through the docs more closely and that description actually had a multi-line git trailer, which our string parsing code was misinterpreting. Sent https://github.com/iree-org/iree/pull/19244 to just ignore a few cases and make the script more robust. We don't make heavy enough use of git trailers to need support for multi-line IMO.

ScottTodd commented 1 day ago

If you message omitted the leading spaces, the parsing would have worked.

This is interpreted as a multi-line git trailer:

Notes:
 - Completely self-contained C avoids including even C standard library headers, so that we don't run into compilation failures on some CI host based on the C headers installed on it (see https://github.com/iree-org/iree/pull/19194#issuecomment-2484137599).

This is interpreted as having no git trailer since the last line not starting with a space did not have a : in it:

Notes:
- Completely self-contained C avoids including even C standard library headers, so that we don't run into compilation failures on some CI host based on the C headers installed on it (see https://github.com/iree-org/iree/pull/19194#issuecomment-2484137599).

(space or no space before the - at the start of the last line)