Proposal: Add mimaReportBinaryIssues to the prePR command

[danicheg]

I noticed that prePR doesn't contain the mimaReportBinaryIssues. Is it intentional? I want to promote using the prePR in several projects, so binary compatibility checking is an important step.

[armanbilge]

Thanks for the PR. I agree, I would also like prePR to be as helpful as possible.

I think my biggest reservation about mimaReportBinaryIssues is that it requires downloading lots and lots of old artifacts. Maybe this is not a big deal, not sure.

The fact that it currently involves a clean recompile is also something I've felt a bit weird about. In fact, I frequently use the tlPrePrBotHook command designed for steward since it's much lighter-weight :)

Would be really great to get feedback on this from others!

[danicheg]

I think my biggest reservation about mimaReportBinaryIssues is that it requires downloading lots and lots of old artifacts. Maybe this is not a big deal, not sure.

If prePR is supposed to be used actually before submitting the PR, I believe it's worth the hassle. It just saves some time for getting feedback (instead of waiting for the CI and having several iterations).

The fact that it currently involves a clean recompile is also something I've felt a bit weird about.

I feel that the prePR command should contain almost all the steps that the CI job does. In the case of the MiMa tool, it may give some good tips to contributors before submitting the PR. But yeah, it's always a compromise between time and completeness.

[danicheg]

Btw one more thing I could advise is to get rid of setting the root project at the initial of the prePR command. It would be wasteful for those who made changes in only one submodule.

[armanbilge]

I feel that the prePR command should contain almost all the steps that the CI job does.

I think we can instead offer a ci command which is a more faithful reproduction, see #202.

I believe it's worth the hassle

Idk, I feel really bad about downloading all those artifacts onto contributors locals.

Btw one more thing I could advise is to get rid of setting the root project at the initial of the prePR command.

Oh yeah, that's probably a good idea.

[armanbilge]

Btw, I asked for feedback on Discord #general, in case you want to have a look.

[danicheg]

So, after a deep conversation on the topic, I think it's needed to work in another direction. My advice is to divide prePR by intentions - "polish" (scalafmt/scalafix/headers) and things that check one don't break something (tests/mima/fatal-warnings).

[rossabaker]

That sounds fine, but note that some scalafix is "polish" (e.g., organize imports), and some "break something" (e.g., banning Sync stream compiler). Moving to the open issue.