Skip to content

Fix oss-check for pull requests that change only rules - #6934

Open
lechuckcaptain wants to merge 1 commit into
realm:mainfrom
lechuckcaptain:fix-oss-check-only-rules
Open

lechuckcaptain wants to merge 1 commit into
realm:mainfrom
lechuckcaptain:fix-oss-check-only-rules

Conversation

@lechuckcaptain

Copy link
Copy Markdown

Fixes #6933.

When a pull request changes only rule files, oss-check adds an only_rules list to each project's config. Brigade and SwiftLintPerformanceTests keep their own configs, and both use opt_in_rules. SwiftLint does not accept both keys in one file:

warning: 'disabled_rules' or 'opt_in_rules' cannot be used in combination with 'only_rules' – Falling back to default configuration

So SwiftLint lints those two projects with the default rules and without their excluded: lists. It parses Brigade's crash reproducers, and a release build crashes with no report. JSON.parse fails on the empty report, and the Danger step stops with no comment.

This pull request makes two changes:

  • Pass --only-rule for each changed rule instead of editing the config. The flag ignores only_rules, opt_in_rules and disabled_rules, so it cannot conflict with a project's config. Each project also keeps its excluded: list. SwiftLint selects rules for --only-rule and only_rules in the same code, in Configuration+RulesWrapper.swift. So the other 17 projects get the same results as before.
  • Check each report before parsing it. If a report is empty, show a warning with the project name and skip that project. This check runs only when both builds have the same exit value. So an empty report means both builds failed, and the pull request did not cause it.

Validation

This pull request changes no rule file, so its own CI does not run the changed code. I ran it locally from a branch that changes one rule file:

tools/oss-check -v --force --only-repos Aerial,Brigade
before after
exit code 1, JSON::ParserError 0
Brigade.json 0 bytes 1.1 MB, 3552 violations
Brigade results none, the script stopped reported

With the fix, oss-check also reports the violations that the rule change fixes in Brigade, for example in Sources/BrigadeApp/Plating.swift.

Overlap with #6826

The draft #6826 moves the same only_rules block in setup_repos. It keeps the append, so it does not fix this. The two pull requests will conflict in that block. I can rebase this one if #6826 lands first.

When a pull request changes only rule files, oss-check runs just those
rules. It did this by adding an `only_rules` list to each project's
`.swiftlint.yml`.

That breaks two projects, Brigade and SwiftLintPerformanceTests. oss-check
keeps their own configs, and both use `opt_in_rules`. SwiftLint does not
accept `only_rules` and `opt_in_rules` in the same file.

SwiftLint then falls back to its default configuration. It lints those two
projects with the default rules instead of the changed ones, so their
results are wrong. The default configuration also has no `excluded:` list,
so SwiftLint parses Brigade's crash reproducers. A release build overflows
its stack on one of them and writes no report. oss-check then parses the
empty report and crashes, and the Danger step fails with no comment.

Pass `--only-rule` on the command line instead. You can repeat the flag, and
it ignores `only_rules`, `opt_in_rules` and `disabled_rules`. A project's own
config cannot conflict with it, and each project keeps its `excluded:` list.
SwiftLint selects rules for `--only-rule` and `only_rules` in the same code,
so every other project gets the same results as before.

Also check each report before parsing it. If a report is empty, show a
warning with the project name and skip that project. Before, an empty report
caused a JSON parse error that hid the real problem.
@lechuckcaptain
lechuckcaptain marked this pull request as ready for review September 27, 2026 09:49
@SwiftLintBot

Copy link
Copy Markdown
20 Messages
📖 Building this branch resulted in the same binary size as when built on main.
📖 Linting Aerial with this PR took 0.68 s vs 0.69 s on main (1% faster).
📖 Linting Alamofire with this PR took 0.98 s vs 0.98 s on main (0% slower).
📖 Linting Brave with this PR took 5.55 s vs 5.56 s on main (0% faster).
📖 Linting Brigade with this PR took 18.11 s vs 18.1 s on main (0% slower).
📖 Linting DuckDuckGo with this PR took 32.03 s vs 31.96 s on main (0% slower).
📖 Linting Firefox with this PR took 11.76 s vs 11.79 s on main (0% faster).
📖 Linting Kickstarter with this PR took 7.42 s vs 7.44 s on main (0% faster).
📖 Linting Moya with this PR took 0.38 s vs 0.37 s on main (2% slower).
📖 Linting NetNewsWire with this PR took 2.58 s vs 2.6 s on main (0% faster).
📖 Linting Nimble with this PR took 0.56 s vs 0.56 s on main (0% slower).
📖 Linting PocketCasts with this PR took 7.48 s vs 7.52 s on main (0% faster).
📖 Linting Quick with this PR took 0.36 s vs 0.37 s on main (2% faster).
📖 Linting Realm with this PR took 2.97 s vs 2.98 s on main (0% faster).
📖 Linting Sourcery with this PR took 1.63 s vs 1.61 s on main (1% slower).
📖 Linting Swift with this PR took 4.59 s vs 4.61 s on main (0% faster).
📖 Linting SwiftLintPerformanceTests with this PR took 0.18 s vs 0.18 s on main (0% slower).
📖 Linting VLC with this PR took 1.44 s vs 1.45 s on main (0% faster).
📖 Linting Wire with this PR took 16.59 s vs 16.59 s on main (0% slower).
📖 Linting WordPress with this PR took 10.69 s vs 10.74 s on main (0% faster).

Generated by 🚫 Danger

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

oss-check writes an invalid config for projects that keep their own .swiftlint.yml

2 participants