Repro: version compatibility changes failure

[ylecornec]

In the current develop branch running dune build --profile=nonconsensus_mainnet src/nonconsensus/snark_params fails with

File "src/nonconsensus/snark_params/tick.ml", line 43, characters 11-16:
43 |     module Tests = struct end
                ^^^^^
Error: Versioning module containing versioned type must be named Vn, for some number n

The CI only does this (in the version compatibility changes) test if we modify the tick.ml file, so it may have been unnoticed until PR #11558. Moreover fixing it may require forcing the merge on red CI, because the version compatibility changes test tries to compile the current develop branch which contains the bug.

Repro

The add comment to tick.ml commit below only adds a comment and CI fails here.

The remove test module commit below removes the Test module which fixes the error locally (I do not know if we really want to do this). However the CI still fail when compiling the current develop branch here.

[psteckler]

You want to remove the Test module. That module used to be required if the Stable module had %%versioned_asserted, but now is prohibited.

[psteckler]

The linter in ppx_version is called when running the print_versioned_types tool, which is why the error occurs in CI.

The tool is called only on files that have changed in a PR, which is why the error had not appeared before.

[psteckler]

Are the nonconsensus libraries used anywhere? I don't think so, which is why this error had not appeared before.

Update: they're not, but they may be pressed into service in the future, per Matthew.

[kantp]

The linter in ppx_version is called when running the print_versioned_types tool, which is why the error occurs in CI.

The tool is called only on files that have changed in a PR, which is why the error had not appeared before.

@psteckler Is my understanding correct that when we introduced the version compatibility changes check, we did not run that check for the whole codebase? And so we now have some files that do not pass that check left in the codebase?

I think we should change that situation, by either making the whole codebase conform to the check, or by disabling it. It might be most pragmatic to disable it now, and make a note to reintroduce it while making the whole code pass later.

How important is the version compatibility changes check? Is it an aesthetic thing, something that where code does not adhere, it's easier to make mistakes, or something that is a bug in itself?

[mrmr1993]

The issue has been resolved, this reproduction is no longer necessary. Closing.