Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ci): assert that patches apply exactly #508

Merged
merged 3 commits into from
Jan 24, 2025
Merged

Conversation

alexeagle
Copy link
Member

@alexeagle alexeagle commented Jan 24, 2025

From man patch

     -F max-fuzz, --fuzz max-fuzz
             Sets the maximum fuzz factor.  This option only applies to
             context diffs, and causes patch to ignore up to that many lines
             in looking for places to install a hunk.  Note that a larger fuzz
             factor increases the odds of a faulty patch.  The default fuzz
             factor is 2, and it may not be set to more than the number of
             lines of context in the context diff, ordinarily 3.

The hint was in our CI job which passes but prints

Run patch --dry-run -p1 < .bcr/patches/*.patch
checking file MODULE.bazel
Hunk #1 succeeded at 45 with fuzz 2 (offset 4 lines).
Hunk #2 succeeded at 61 with fuzz 2 (offset 3 lines).

When Bazel or the Publish to BCR app apply the patches, they don't use the patch tool so they don't allow any variation in the patch lines. Our assertion needs to be strict.

Fixes #506

From `man patch`

```
     -F max-fuzz, --fuzz max-fuzz
             Sets the maximum fuzz factor.  This option only applies to
             context diffs, and causes patch to ignore up to that many lines
             in looking for places to install a hunk.  Note that a larger fuzz
             factor increases the odds of a faulty patch.  The default fuzz
             factor is 2, and it may not be set to more than the number of
             lines of context in the context diff, ordinarily 3.
```

The hint was in our CI job which passes but prints 

```
Run patch --dry-run -p1 < .bcr/patches/*.patch
checking file MODULE.bazel
Hunk #1 succeeded at 45 with fuzz 2 (offset 4 lines).
Hunk #2 succeeded at 61 with fuzz 2 (offset 3 lines).
```

When Bazel or the Publish to BCR app apply the patches, they don't use the `patch` tool so they don't allow any variation in the patch lines. Our assertion needs to be strict.
Copy link

github-actions bot commented Jan 24, 2025

LCOV of commit 13bcc57 during CI #1609

Summary coverage rate:
  lines......: 100.0% (2 of 2 lines)
  functions..: 100.0% (1 of 1 function)
  branches...: no data found

Files changed coverage rate: n/a

Copy link

aspect-workflows bot commented Jan 24, 2025

Test

All tests were cache hits

28 tests (100.0%) were fully cached saving 54s.

@alexeagle alexeagle enabled auto-merge (squash) January 24, 2025 18:06
@alexeagle alexeagle requested review from kormide and thesayyn January 24, 2025 18:06
@hofbi
Copy link
Contributor

hofbi commented Jan 24, 2025

With the excat patch applied, we don't need this line anymore, since not .orig file is produced

alexeagle added a commit to bazel-contrib/.github that referenced this pull request Jan 24, 2025
See aspect-build/rules_py#508 where the absence of this flag allowed a broken rules_py release
@alexeagle
Copy link
Member Author

@hofbi yes that's one option. However that's not the principled thing to do, since the goal of that line is to restore the MODULE file to undo the patching operation. Instead I scanned through man patch again to find the --backup option to make sure we always get that .orig file.

Really important in these rulesets to be precise about what's the root problem and not make workarounds :)

@alexeagle
Copy link
Member Author

Also I don't understand why this test was passing before there was a "mismatch" introduced. You would expect it didn't create .orig files a week ago either.

@alexeagle alexeagle merged commit a709ed8 into main Jan 24, 2025
17 checks passed
@alexeagle alexeagle deleted the alexeagle-patch-4 branch January 24, 2025 18:25
@hofbi
Copy link
Contributor

hofbi commented Jan 24, 2025

Also I don't understand why this test was passing before there was a "mismatch" introduced. You would expect it didn't create .orig files a week ago either.

Yep, that's strange. I tried both the one introducing the mismatch 1d6cd38 and the one before e618b75, and both applied and produce the .orig file. Are we looking at the wrong place?

@alexeagle
Copy link
Member Author

I'm moving on from this but I agree it would be better to understand what happened so we are sure we fixed the root cause

alexeagle added a commit to bazel-contrib/.github that referenced this pull request Jan 24, 2025
See aspect-build/rules_py#508 where the absence of this flag allowed a broken rules_py release
@hofbi
Copy link
Contributor

hofbi commented Jan 24, 2025

After some further history checking, I found that #429 disabled the release test while #496 enabled it again. #417 was the PR that made it red and updated the patch. Unfortunelty it is too long ago to check the CI log and verify if the error was the missing .orig file back then. Maybe there were 2 errors that compensated each other 🤷 . But seems like we won't find out without the CI logs, so let's move on.

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.

[Bug]: aspect_rules_py 1.2.0 cannot be downloaded from BCR
3 participants