Rewrite check_toml_lints script in python to fix bug#1488
Rewrite check_toml_lints script in python to fix bug#1488tmpolaczyk wants to merge 3 commits intomasterfrom
Conversation
It used to always return 0 because of how bash subshells interact with variables.
📝 WalkthroughWalkthroughAdds a Python script that scans tracked Cargo.toml files (via Git), validates each contains a [lints] table with workspace = true, replaces the removed shell script in the release workflow, and updates one proc-macro Cargo.toml to include the lints setting. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Script as check_toml_lints.py
participant Git as Git CLI
participant FS as Filesystem
participant TOML as tomllib
CI->>Script: invoke script
Script->>Git: git rev-parse --show-toplevel
Git-->>Script: repo root path
Script->>Git: git ls-files -z '**/Cargo.toml'
Git-->>Script: NUL-separated paths
loop per manifest (excluding repo root)
Script->>FS: open manifest path
alt IO error
FS-->>Script: error
Script->>CI: emit read error for path
else opened
FS-->>Script: file bytes
Script->>TOML: parse bytes
alt parse error
TOML-->>Script: decode error
Script->>CI: emit parse error for path
else parsed
TOML-->>Script: toml dict
Script->>Script: check lints.workspace == true
alt missing/false
Script->>CI: emit "Missing [lints] workspace = true in <path>"
else valid
Script-->>CI: no issue
end
end
end
end
Script->>CI: exit 0 if all valid else exit 1
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/scripts/check_toml_lints.py:
- Around line 162-164: The "Missing [lints] workspace = true" message is printed
whenever ok is False, which wrongly shows when the file failed to be
read/parsed; change the logic to distinguish read/parse errors from a genuine
missing configuration: introduce a separate flag (e.g., read_ok or parsed_ok)
when opening/parsing the TOML succeeds, keep using ok for "has required config",
and only print the "Missing [lints] workspace = true in {rel}" message (and set
failure=1) when parsed_ok is True but ok is False; update the code paths around
the file open/parse and the if not ok block to check both flags accordingly,
referencing ok, parsed_ok (or read_ok), failure, and rel.
In @.github/workflows/release.yml:
- Around line 116-117: The workflow step that runs check_toml_lints.py assumes
Python 3.11+ because the script imports tomllib; update the release.yml workflow
to ensure a compatible Python version is available by adding a setup step (e.g.,
actions/setup-python@v4 with python-version: '3.11') before the step that runs
./.github/scripts/check_toml_lints.py or alternatively document/validate that
the self-hosted runner provides Python >=3.11; reference the script name
check_toml_lints.py and the tomllib import to locate where this requirement
originates.
🧹 Nitpick comments (1)
.github/scripts/check_toml_lints.py (1)
13-13: Consider using straight quotes in docstrings for consistency.The docstring uses curly/smart quotes (
'cargo clippy') instead of straight quotes. While this doesn't affect functionality, using straight quotes (`cargo clippy`) would be more consistent with typical Python documentation conventions.
| if not ok: | ||
| print(f"Missing [lints] workspace = true in {rel}") | ||
| failure = 1 |
There was a problem hiding this comment.
Error message may be misleading when file access fails.
When ok is False due to a FileNotFoundError, PermissionError, or TOMLDecodeError, the code already prints an appropriate error message (e.g., "Error: file not found"). However, it then also prints "Missing [lints] workspace = true", which is misleading since the actual problem is file access or parsing, not a missing configuration.
Consider only printing the "Missing" message when the file was successfully read but lacks the required configuration.
🛠️ Suggested fix
except OSError as oe:
print(f"Error: could not read {rel}: {oe}", file=sys.stderr)
ok = False
+ failure = 1
+ continue
if not ok:
print(f"Missing [lints] workspace = true in {rel}")
failure = 1Alternatively, track whether we already printed an error:
- except FileNotFoundError:
- print(f"Error: file not found: {rel}", file=sys.stderr)
- ok = False
- except PermissionError:
- print(f"Error: permission denied reading: {rel}", file=sys.stderr)
- ok = False
- except tomllib.TOMLDecodeError as te:
- print(f"Error: invalid TOML in {rel}: {te}", file=sys.stderr)
- ok = False
- except OSError as oe:
- print(f"Error: could not read {rel}: {oe}", file=sys.stderr)
- ok = False
-
- if not ok:
- print(f"Missing [lints] workspace = true in {rel}")
- failure = 1
+ except (FileNotFoundError, PermissionError, tomllib.TOMLDecodeError, OSError) as e:
+ print(f"Error reading {rel}: {e}", file=sys.stderr)
+ failure = 1
+ else:
+ if not ok:
+ print(f"Missing [lints] workspace = true in {rel}")
+ failure = 1🤖 Prompt for AI Agents
In @.github/scripts/check_toml_lints.py around lines 162 - 164, The "Missing
[lints] workspace = true" message is printed whenever ok is False, which wrongly
shows when the file failed to be read/parsed; change the logic to distinguish
read/parse errors from a genuine missing configuration: introduce a separate
flag (e.g., read_ok or parsed_ok) when opening/parsing the TOML succeeds, keep
using ok for "has required config", and only print the "Missing [lints]
workspace = true in {rel}" message (and set failure=1) when parsed_ok is True
but ok is False; update the code paths around the file open/parse and the if not
ok block to check both flags accordingly, referencing ok, parsed_ok (or
read_ok), failure, and rel.
| - name: Find toml files with lints key not set | ||
| run: ./.github/scripts/check_toml_lints.sh | ||
| run: ./.github/scripts/check_toml_lints.py |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the release.yml workflow around the flagged lines
cat -n .github/workflows/release.yml | sed -n '100,130p'Repository: moondance-labs/tanssi
Length of output: 1640
🏁 Script executed:
# Check if the check_toml_lints.py script exists and what it contains
if [ -f ".github/scripts/check_toml_lints.py" ]; then
echo "=== check_toml_lints.py found ==="
cat -n .github/scripts/check_toml_lints.py
else
echo "check_toml_lints.py not found"
fiRepository: moondance-labs/tanssi
Length of output: 6912
🏁 Script executed:
# Check for any Python version setup in the workflow and surrounding context
cat -n .github/workflows/release.yml | head -50Repository: moondance-labs/tanssi
Length of output: 2261
🏁 Script executed:
# Look for any python or setup-python references in the entire workflow
rg -n "python|Python" .github/workflows/release.ymlRepository: moondance-labs/tanssi
Length of output: 47
Ensure self-hosted runner has Python 3.11+ or add version requirement to workflow.
The check_toml_lints.py script explicitly requires Python 3.11+ for the tomllib module (line 37), with no fallback implementation. The workflow contains no Python version setup step, so if the self-hosted runner's default Python is 3.10 or earlier, this step will fail with an ImportError when trying to import tomllib.
Either ensure the runner has Python 3.11+ available, or add a setup step to guarantee the correct version (e.g., using actions/setup-python@v4 with python-version: '3.11').
🤖 Prompt for AI Agents
In @.github/workflows/release.yml around lines 116 - 117, The workflow step that
runs check_toml_lints.py assumes Python 3.11+ because the script imports
tomllib; update the release.yml workflow to ensure a compatible Python version
is available by adding a setup step (e.g., actions/setup-python@v4 with
python-version: '3.11') before the step that runs
./.github/scripts/check_toml_lints.py or alternatively document/validate that
the self-hosted runner provides Python >=3.11; reference the script name
check_toml_lints.py and the tomllib import to locate where this requirement
originates.
WASM runtime size check:Compared to target branchdancebox runtime: 1900 KB (no changes) ✅ flashbox runtime: 1124 KB (no changes) ✅ dancelight runtime: 2668 KB (no changes) 🚨 starlight runtime: 2584 KB (no changes) 🚨 container chain template simple runtime: 1516 KB (no changes) ✅ container chain template frontier runtime: 1848 KB (no changes) ✅ |
Coverage Report@@ Coverage Diff @@
## master tomasz-toml-lints-python +/- ##
============================================================
- Coverage 73.91% 73.84% -0.07%
- Files 569 568 -1
- Lines 83085 82805 -280
============================================================
- Hits 61410 61141 -269
- Misses 21675 21664 -11
|
1 participant
It used to always return 0 because of how bash subshells interact with variables.