How do we merge compathelper PRs?

[clizbe]

This is a n00b question, but when compathelper (or any other bot) opens a PR, what do we do before merging? Just wait till the tests pass and nothing is blocking merge except review? Or is there more stuff to check?
This is something I worry is only known by a couple of people and isn't documented for the future (n00b) devs like me.

@suvayu @abelsiqueira

[suvayu]

I review the PRs just like any other PR. Depending on the content, I accept or reject them. E.g. I remember for TIO, I rejected a DuckDB compat entry because I wanted the next version (1.0.0) whereas the compat entry restricted it to 0.10.

[clizbe]

But how do you know what you want?

[suvayu]

All of these automated PRs do pretty simple stuff right? E.g. CompatHelper just adds compat entries. So for each of these contexts, I have something in mind. In the above example, I knew I want to go to 1.0, so I rejected it. Similarly, if you look at TIO now, I think there's a CompatHelper PR (something DuckDB related), most likely I'll reject it because I want manual control because of this "cannot find symbol" issue on Windows.

That's why I said "review like any other PR". Just like any other PR, you read it, and decide based on the contents. Does that help?

[abelsiqueira]

What @suvayu said. In particular, the CompatHelper auto PR has this message:

Note: I have not tested your package with this new compat entry.
It is your responsibility to make sure that your package tests pass before you merge this pull request.
Note: Consider registering a new release of your package immediately after merging this PR, as downstream packages may depend on this for tests to pass.

This automated PRs cannot trigger tests, so the CompatHelper is essentially a warning "Hey, there's a new version out there that you don't use/support". There are three scenarios:

• GOOD: New version works fine
  • Update compat
• BAD: New version breaks package
  • Either not update compat or fix our package
• UGLY: New version is actually not used because it's constrained by another package (This happens if other packages in our compat actually depend on that new package but haven't been updated)
  • Don't update compat yet

There are a few alternatives to verify the things above, depending on your fault tolerance.

1. Accept PR and see the tests run on main (my fave) - notice that the state of main is always "potentially broken, don't use"
   • If the tests fail, you know it's BAD, since you already merged the commit, either revert, or work on fixing the package
   • If the tests pass, it's either GOOD or UGLY. Verify that the correct versions was installed by either
     • Checking the log that the correct version was actually installed (use search the log)
     • Fetch, run pkg> update, and check your pkg> status
2. Try to trigger the PR by either closing and reopening, or by cloning it locally, amending and force-pushing
   • Same verifications as 1, except that the tests are in the PR branch
3. Clone and test locally and do the same verifications as 1

Since option 1 is GOOD most of the time, the burden of switching hasn't been justified yet (depends on the project and number of dependencies)

[clizbe]

Thanks @abelsiqueira! @suvayu had the What, but I was asking about the How.
I don't really get what you mean by triggering the PR in 2. That makes it sound like normal PRs run tests but these don't for some reason - can we get them to run automatically on the PR branch? (Low priority, just curious.)

[abelsiqueira]

Correction on 2. It should be "Trigger the PR's tests/checks". Indeed, these don't run automatically for safety/economical reasons, because they are created by other workflows - for instance, if you create circular workflows that trigger one another, you would have an infinite loop of PRs.
We can't fix this under reasonable assumptions on money, time, and security.