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

Usability issue: versionCheck doesn't show the problems it found #126

Closed
SethTisue opened this issue May 13, 2022 · 10 comments · Fixed by #169
Closed

Usability issue: versionCheck doesn't show the problems it found #126

SethTisue opened this issue May 13, 2022 · 10 comments · Fixed by #169

Comments

@SethTisue
Copy link

I have hit this repeatedly in multiple repos over the past year or two – only getting around to reporting it now.

When versionPolicyCheck fails, I get the usual MiMa output showing me what the actual problems are.

But when versionCheck (the one you run at release time, rather than during PR validation) fails, I don't get any details, I just get e.g.

sbt:root> parserCombinatorsJVM / versionCheck
[error] Module scala-parser-combinators has an invalid version number: 1.1.3-RC1. You must increment the minor version number to publish a source incompatible release.
[error] (parserCombinatorsJVM / versionCheck) Module scala-parser-combinators has an invalid version number: 1.1.3-RC1. You must increment the minor version number to publish a source incompatible release.

according to the repo readme here,

MiMa can only detect binary incompatibilities. To detect source incompatibilities, this plugin uses MiMa in forward mode as an approximation. This is not always correct and may lead to false positives or false negatives. This is a known limitation of the current implementation.

I'm suggesting change the plugin so that the specific incompatibilities found are printed.

but it'd also be okay if I need to run an extra command in order to "use MiMa in forward mode" and see the errors, as long as we document what that command is

@SethTisue
Copy link
Author

looks like I can get the errors by doing set mimaCheckDirection := "forward" followed by mimaReportBinaryIssues

@SethTisue
Copy link
Author

SethTisue commented May 13, 2022

gah, never mind, I misunderstood. the check isn't failing because MiMa is failing — MiMa isn't even running!

rather, it's failing because my build.sbt had Compatibility.BinaryCompatible but I was trying to release a patch version anyway.

I don't know if I like this design, but at least I understand what's going on, now. I was baffled/confused before.

@SethTisue
Copy link
Author

SethTisue commented May 13, 2022

Maybe I don't have the big picture or grok the higher wisdom, but....

I guess what I'd sort of expect here is that my versionPolicyIntention would be computed from version which is computed from my tag name... rather than me being required to manually switch my versionPolicyIntention back and forth between unidirectional and bidirectional checking as my releases switch back and forth between patch versions and minor versions

this overlaps with stuff @nafg has been saying on #119

@nafg
Copy link

nafg commented May 15, 2022

In Slick, I took the opposite approach, which is to compute the version from versionPolicyIntention and git tags. (See https://github.com/slick/slick/blob/main/project/Versioning.scala)

The downside of computing versionPolicyIntention from version is then you have to answer how is version specified. You still have to keep updating that. If I have to update one I'd rather it be versionPolicyIntention because:

  • Fewer information bits to deal with. It's a choice of only 3 options to choose from; the rest can be inferred. In fact you could keep all 3 versions in place with 2 of the 3 commented at any given time, so updating it is just changing which line is uncommented
  • It's the bit of information that's missing from the computer's perspective, that needs a human to supply it. The version on the other hand is just counting; that computers can do fine when given the exact rules.
  • It may need changing less often, since you don't have to deal with adding and removing -SNAPSHOT
  • It's less of a contradiction to sbt-dynver. In fact Slick uses sbt-dynver, and I achieve this by customizing it. sbt-dynver expects to infer the version from git tags, but can you automate creating git tags? If the version can be computed from past tags plus versionPolicyIntention then sure! And if your version is explicitly set then you can too, but then you aren't really using sbt-dynver.

In theory, one could have versionPolicyIntention inferred too, from metadata in commits. For instance in one project of mine, I prefix commit messages with ^, *, or + to indicate the respective version segment it would bump, if any. I then have a release script that computes a git tag to create based on that.

@julienrf
Copy link
Collaborator

julienrf commented May 19, 2022

I don't know if I like this design, but at least I understand what's going on, now. I was baffled/confused before.

I agree that this is can be confusing.

The reason why we did that is because we wanted to support as many use cases as possible.

With a typical setup with sbt-dynver, the versionPolicyIntention can not be computed from the version because it looks like 1.2.3+42-1234abcd (unless you push a tag, but this is never the case when you submit a PR, for instance), where 1.2.3 is the last published version. So, sbt-dynver never bumps the patch, minor, or major version, it only appends metadata to the last published version.

We could decide to support sbt-release only, and in this case we would not have had to introduce the versionPolicyIntention at all, since with sbt-release the version is managed manually (we could derive versionPolicyIntention from it, or derive version from versionPolicyIntention, as you prefer, because only one of both keys is needed). That being said, I think we should support the sbt-dynver setup because I believe it’s quite pervasive (especially because of sbt-ci-release).

I hope this clarifies things.

@SethTisue
Copy link
Author

it does, though in 6 months I'll probably have forgotten it and confuse myself all over again :-)

@SethTisue
Copy link
Author

SethTisue commented Jul 14, 2023

the unhelpfulness of the error message bit Lukas today over at scala/scala-xml#685 (comment)

it does, though in 6 months I'll probably have forgotten it and confuse myself all over again :-)

correct prediction, it took me about 20 minutes to figure it out

@SethTisue
Copy link
Author

Julien asks, over on the scala-xml ticket:

Do you have suggestions for a better error message?

The main thing would be for versionCheck to show me the actual MiMa problems it found, as in the original title of this ticket. (Perhaps I shouldn't have closed this ticket — feel free to reopen.) Without that, it isn't even clear that there are any MiMa failures involved...!

Additionally, it would be helpful if the error message showed the versionPolicyIntention and the old and new version numbers, to help the user sanity-checking that the values they are intended are the values being used.

at present, we see e.g.

[error] Module scala-xml has an invalid version number: 1.3.1. You must increment the minor version number to publish a source incompatible release.

How about something more like:

[error] Module scala-xml has a version number (now 1.3.1, previously 1.3.0) inconsistent with its declared versionPolicyIntention of BinaryCompatible. This intention implies that the minor version number should be incremented.

@SethTisue
Copy link
Author

oh, I basically already said this above:

I'm suggesting change the plugin so that the specific incompatibilities found are printed.

but it'd also be okay if I need to run an extra command in order to "use MiMa in forward mode" and see the errors, as long as we document what that command is

@julienrf
Copy link
Collaborator

Note that versionCheck does not perform any binary compatibility checks, it only checks that the declared version conforms to the declared versionPolicyIntention.

In #169 I change the reporting to show the following message instead:

Module version-test has a declared version number 1.0.2 that does not conform to its declared versionPolicyIntention of BinaryCompatible. If you want to publish a source incompatible release, you must increment the minor version number. Otherwise, to release version 1.0.2, you need to restrict the versionPolicyIntention to BinaryAndSourceCompatible and run versionPolicyCheck again.

Module version-test has a declared version number 1.2.0 that does not conform to its declared versionPolicyIntention of None. If you want to publish a binary incompatible release, you must increment the major version number (or the minor version number, if the major version is 0). Otherwise, to release version 1.2.0, you need to restrict the versionPolicyIntention to BinaryCompatible and run versionPolicyCheck again.

You can see the full logs here.

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 a pull request may close this issue.

3 participants