Implement skip list for version compatibility lint step

[ebresafegaga]

Implements the skip list for #11780, and closes it. This will help to unblock #11558.

[psteckler]

There's a plan for the version linter, not yet implemented, which will get rid of most of the linter noise. The compare_versioned_types scripts is going away.

[ebresafegaga]

There's a plan for the version linter, not yet implemented, which will get rid of most of the linter noise. The compare_versioned_types scripts is going away.

But until then, maybe this could be a temporary fix to avoid blockers with some new PRs?

[kantp]

There's a plan for the version linter, not yet implemented, which will get rid of most of the linter noise. The compare_versioned_types scripts is going away.

Thanks, that's great to know! We should make a decision for what to do in the meantime, until we have the new version linter. I would advocate for implementing the skip list, to avoid blocking PRs like #11558 and #11629. Alternatively, we could also disable the check until we have the new version linter, but I think that would be going too far.

[psteckler]

I'm not sure I understand the approach here.

A type doesn't pass or fail the linter in isolation. A type fails the linter if it's different in the PR branch from the same type in the PR base branch.

You might not want to issue a warning if the type is not in use on mainnet. In that situation, users are not depending on the serialization of that type. But the current setup doesn't allow use to identify which types are in use on mainnet, so the linter gives undesired noise. The upcoming changes will do that, by checking against a "release" branch.

When such noisy warnings occur, we've been manually merging PRs, after examining the contents of the linter warning. For example, changes to zkApps-related types, which are not in use on mainnet, can be merged despite linter warnings. It happens, but not super often.

I'm also worried that the approach here is too coarse-grained: a file might contain types that you want to lint, as well as types that you don't want to lint.

[kantp]

Ok, let me explain the reasoning behind this proposed change: to my understanding, there are currently some files that do not pass the linter, which have not been detected, because the linter is only run when files are changed. This causes CI to fail on PRs if they make some change within such a file (as in #11558). Is that understanding correct?

If it is, the ideal solution would be run the linter on all files, and edit those that fail to pass the linter. But that would be a lot of work, and it's not clear to me that it's a thing we need to do right now (but maybe there is?). So, as a stopgap, the skip list would allow us to say "this file doesn't pass the linter, but it never did, so CI should not block PRs modifying the file", and remove the blockage.

Alternatively, I would also be fine with force merging PRs such as #11558 (though I would still like the error message in the linter to refer to this possibility, so that people are not surprised by CI failing on something that is unrelated to their PR).

[bkase]

If we (a) make this file scripts/compare_versioned_types.py have a strict CODEOWNERS so we don't accidentally add bad files (delegate to @joseandro for deciding which person(s) or teams should be on that) and (b) only add files that we haven't touched since adding the linter (which maybe we can add in a comment above the list), then this seems safe to me

[bkase]

The comments look good to me but we need the CODEOWNERS change too, I’ll ask @joseandro to comment again

[kantp]

!ci-build-me

[psteckler]

The updated version linter task is described in detail here:

https://github.com/MinaProtocol/mina/issues/11181#issuecomment-1278106386

It might be worth starting on that, so this PR is not needed.

[mrmr1993]

The scripts that this modifies have been removed and replaced with the approach described in #11181. Closing.