Introduce task versionPolicyAssessCompatibility #184

julienrf · 2023-11-27T15:13:59Z

This is another attempt to address #179 / #109. It should also help to address #121.

We introduce a bunch of new keys:

versionPolicyFindMimaIssues, which returns all the Mima issues found on the project against its previous versions (similar to the existing versionPolicyFindDependencyIssues)
versionPolicyFindIssues, which returns both Mima issues and dependency issues
versionPolicyAssessCompatibility, which assess the compatibility level of the project based on the result of versionPolicyFindIssues.

Unfortunately, I couldn’t find a way to implement these changes without breaking the binary compatibility. On the other hand, breaking the binary compatibility was a good opportunity to clean up the code base:

I removed a couple of unused keys (versionPolicyForwardCompatibilityCheck and versionPolicyCheckDirection)
I removed the dependency to data-class
I simplified the implementation of DependencyCheckReport (so Facilitate using the plugin for custom reports #121 should be easier to achieve)

I also updated the documentation.

This work is sponsored by The Guardian ❤️

rtyley · 2023-11-28T16:13:39Z

I tried this out (with 2.1.3+12-ac7a2944-SNAPSHOT) on a sample library (guardian/etag-caching), and got some good data out of it when I tried making various modifications to the library:

No changes (or adding a `println` to the middle of a method implementation)

show core/versionPolicyAssessCompatibility
[info] compiling 1 Scala source to /Users/Roberto_Tyley/code/etag-caching/core/target/scala-2.13/classes ...
[info] * (com.gu.etag-caching:core:1.0.7,BinaryAndSourceCompatible)

Adding a new method

show core/versionPolicyAssessCompatibility
[info] compiling 1 Scala source to /Users/Roberto_Tyley/code/etag-caching/core/target/scala-2.13/classes ...
[info] * (com.gu.etag-caching:core:1.0.7,BinaryCompatible)

Removing an existing method

show core/versionPolicyAssessCompatibility
[info] compiling 1 Scala source to /Users/Roberto_Tyley/code/etag-caching/core/target/scala-2.13/classes ...
[info] * (com.gu.etag-caching:core:1.0.7,None)

One thing that was sort of interesting was that I did have to specify specific subprojects (eg core in the sample above). If I tried to use just show versionPolicyAssessCompatibility I got this error:

sbt:etag-caching-root> show versionPolicyAssessCompatibility
[info] Updating
[info] Resolved  dependencies
[info] Updating
[info] Resolved  dependencies
[info] compiling 1 Scala source to /Users/Roberto_Tyley/code/etag-caching/aws-s3/aws-sdk-v2/target/scala-2.13/classes ...
[error] stack trace is suppressed; run last mimaPreviousClassfiles for the full output
[error] stack trace is suppressed; run last versionPolicyFindDependencyIssues for the full output
[error] (mimaPreviousClassfiles) sbt.librarymanagement.ResolveException: Error downloading com.gu.etag-caching:etag-caching-root_2.13:1.0.7
[error]   Not found
[error]   not found: /Users/Roberto_Tyley/.ivy2/local/com.gu.etag-caching/etag-caching-root_2.13/1.0.7/ivys/ivy.xml
[error]   not found: https://repo1.maven.org/maven2/com/gu/etag-caching/etag-caching-root_2.13/1.0.7/etag-caching-root_2.13-1.0.7.pom
[error] (versionPolicyFindDependencyIssues) sbt.librarymanagement.ResolveException: Error downloading com.gu.etag-caching:etag-caching-root_2.13:1.0.7
[error]   Not found
[error]   not found: /Users/Roberto_Tyley/.ivy2/local/com.gu.etag-caching/etag-caching-root_2.13/1.0.7/ivys/ivy.xml
[error]   not found: https://repo1.maven.org/maven2/com/gu/etag-caching/etag-caching-root_2.13/1.0.7/etag-caching-root_2.13-1.0.7.pom
[error] Total time: 1 s, completed 28 Nov 2023, 16:06:33

...the reason for this is we have publish / skip := true on for the etag-caching-root project, so we don't publish a com.gu.etag-caching:etag-caching-root_2.13 artifact - consequently mimaPreviousClassfiles is bound to fail when aggregated over whole project.

I wonder if the it would be appropriate for sbt-version-policy tasks to skip if publish / skip := true is set on a project? This will probably also come into play when we try to determine the correct version number for a project to update - we need to be looking at the compatibility of everything aggregated by the top-level project, and using the lowest level of compatibility across all of them to determine the required version increment.

julienrf · 2023-11-28T18:31:26Z

Thank you for testing, @rtyley!

I wonder if the it would be appropriate for sbt-version-policy tasks to skip if publish / skip := true is set on a project?

Good point. I will have a look tomorrow.

julienrf · 2023-11-29T08:25:28Z

I wonder if the it would be appropriate for sbt-version-policy tasks to skip if publish / skip := true is set on a project?

Done in 700cfa1.

We can reduce switch statements, and line count a little bit too, if we use a `Map[IncompatibilityType, ...]` rather than two separate `binaryCompatibilityReport` & `sourceCompatibilityReport` fields. This is proposed change for scalacenter#184

rtyley

The output from this is looking good, though working out how to get a GitHub Workflow to consume the output is still a little tricky - this is the kind of output we're currently getting:

sbt:etag-caching-root> show versionPolicyAssessCompatibility
[info] core / versionPolicyAssessCompatibility
[info] 	Vector((com.gu.etag-caching:core:1.0.7,BinaryCompatible))
[info] aws-s3-base / versionPolicyAssessCompatibility
[info] 	Vector((com.gu.etag-caching:aws-s3-base:1.0.7,BinaryAndSourceCompatible))
[info] aws-s3-sdk-v2 / versionPolicyAssessCompatibility
[info] 	Vector((com.gu.etag-caching:aws-s3-sdk-v2:1.0.7,BinaryCompatible))
[info] versionPolicyAssessCompatibility
[info] 	List()

If there was some way of outputting a summary like this, it would probably be ideal for consuming from a GitHub workflow:

comparedWithVersion: 1.0.7
overallCompatibility: BinaryCompatible

It might be that we want this output into a file, rather than the console, to make it easier to consume. I realise I'm potentially asking for non-generic things here, just wanted to try and work out where we need to get to...

Apart from that, and bearing in mind that I'm not an expert in this code and won't know all the crucial nuances, I did add a query about ModuleID, and offered a couple of small tweaks to code if you want them!

rtyley · 2023-11-29T11:03:19Z

sbt-version-policy/src/main/scala/sbtversionpolicy/SbtVersionPolicySettings.scala

+        val dependencyIssues = versionPolicyFindDependencyIssues.value
+        val mimaIssues = versionPolicyFindMimaIssues.value
+        assert(
+          dependencyIssues.map(_._1.revision).toSet == mimaIssues.map(_._1.revision).toSet,
+          "Dependency issues and Mima issues must be checked against the same previous releases"
+        )
+        for ((previousModule, dependencyReport) <- dependencyIssues) yield {
+          val (_, problems) =
+            mimaIssues
+              .find { case (id, _) => previousModule.revision == id.revision }
+              .get // See assertion above
+          previousModule -> (dependencyReport, problems)
+        }


I'm having a bit of trouble understanding this code, I think because I'm not sure exactly what range of values it's operating over. dependencyIssues & mimaIssues are both Seq[(sbt.ModuleID, ...] but what range of ModuleID do we expect that to be? I assume that ModuleID means a subproject within the sbt project we are running in, but I'm not sure of:

will all ModuleIds be for the same module, but potentially for many different revisions? Would it be reasonable to assert that we're only expecting one common revision?

if ModuleIds can be for differing modules (eg core & extra-thing), then linking them together just by revision (previousModule.revision == id.revision) wouldn't work, right?

That’s a great question. The module IDs we iterate over are the previous versions of the same module that we check the compatibility against.

It is possible to check the compatibility against several previous versions, not just one, although by default only the previous release is tested. This can be customized by the users via the key versionPolicyPreviousVersions.

We inherited this behavior because Mima itself works like that, although I am not sure it is that important that we follow the same approach in sbt-version-policy. I decided not to change that in this PR.

So, ideally we’d have a range of revisions, not a range of module IDs, but versionPolicyFindDependencyIssues already used a range of module IDs so I followed the same structure for versionPolicyFindMimaIssues as well (and also versionPolicyFindIssues and versionPolicyAssessCompatibility…).

Ah, thanks for explaining that! It does make me wonder whether versionPolicyAssessCompatibility, being a new key, needs to return Seq[(ModuleID, Compatibility)], or whether it could be just (ModuleID, Compatibility), giving info for the latest published revision. Anything that makes the output simpler to model and easier to parse is good from my perspective - would it be harmful to sbt-version-policy to make that simplification on this new key?

I see two arguments to keep the Seq:

it allows the users to compute the compatibility against multiple previous versions if they want to (although I have no clue whether people really need that…), without adding much costs on our side.

we currently implement the skip behavior by returning an empty collection. Alternatively, we could use an Option. A better solution would be to “assign” the task to its definition (the part where we do versionPolicyAssessCompatibility := { … }) only if skip is false, but I am not sure sbt supports that.

I think the skip argument is more compelling for me, though it is a bit unfortunate it's necessary 😞 Never mind!

sbt-version-policy/src/main/scala/sbtversionpolicy/SbtVersionPolicySettings.scala

sbt-version-policy/src/main/scala/sbtversionpolicy/DependencyCheckReport.scala

We can reduce switch statements, and line count a little bit too, if we use a `Map[IncompatibilityType, ...]` rather than two separate `binaryCompatibilityReport` & `sourceCompatibilityReport` fields. This is proposed change for scalacenter#184

julienrf · 2023-11-29T12:07:55Z

Thank you for the review!

working out how to get a GitHub Workflow to consume the output is still a little tricky

I agree. The task versionPolicyAssessCompatibility returns a Scala value and is meant to be used by custom sbt tasks.

In the future, we could add a task such as versionPolicyExportCompatibility which would export the information in a way that could be consumed by downstream tools.

Co-authored-by: Roberto Tyley <roberto.tyley@theguardian.com>

…f skip is true This is a bit dirty because those tasks are expected to return a result. Skipping a task that returns a result means that we still have to return a result somehow (unless we fail the task). Here, we return an empty result.

rtyley · 2023-11-29T12:24:25Z

In the future, we could add a task such as versionPolicyExportCompatibility which would export the information in a way that could be consumed by downstream tools.

Ah cool! Yes, versionPolicyExportCompatibility makes sense.

rtyley

Personally, looks great to me (I'm not an sbt expert!)- especially appreciate the new documentation in the README 👍

Make sbt-ci-release tests pass on JDK 17

fde33bc

julienrf changed the title ~~Work in progress~~ Introduce task versionPolicyAssessCompatibility Nov 28, 2023

julienrf linked an issue Nov 28, 2023 that may be closed by this pull request

[Question] Is it possible to read the current compatibility status of a project? #109

Closed

Introduce task versionPolicyAssessCompatibility

ac7a294

julienrf marked this pull request as ready for review November 28, 2023 18:14

julienrf force-pushed the find-incompatibilities branch from 923f7cb to 700cfa1 Compare November 29, 2023 08:27

rtyley mentioned this pull request Nov 29, 2023

Code tweak: reduce switch statements in DependencyCheckReport julienrf/sbt-version-policy#1

Merged

rtyley reviewed Nov 29, 2023

View reviewed changes

julienrf force-pushed the find-incompatibilities branch from 109465e to b3ad271 Compare November 29, 2023 12:10

julienrf and others added 2 commits November 29, 2023 13:13

Fix implementation of versionPolicyFindDependencyIssues

088577c

Co-authored-by: Roberto Tyley <roberto.tyley@theguardian.com>

julienrf force-pushed the find-incompatibilities branch from b3ad271 to 5c651a5 Compare November 29, 2023 12:16

rtyley approved these changes Nov 29, 2023

View reviewed changes

julienrf merged commit fef2946 into scalacenter:main Nov 29, 2023
2 checks passed

julienrf deleted the find-incompatibilities branch November 29, 2023 13:17

julienrf mentioned this pull request Nov 29, 2023

Support establishing the actual level of compatibility #179

Closed

rtyley mentioned this pull request Nov 29, 2023

Add stub script for compatibility-labelling PRs guardian/etag-caching#22

Merged

rtyley mentioned this pull request Dec 7, 2023

submodules released as über jars (eg from sbt-assembly) always have MissingClassProblems and None compatibility #190

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce task versionPolicyAssessCompatibility #184

Introduce task versionPolicyAssessCompatibility #184

julienrf commented Nov 27, 2023 •

edited

rtyley commented Nov 28, 2023

julienrf commented Nov 28, 2023 •

edited

julienrf commented Nov 29, 2023 •

edited

rtyley left a comment •

edited

rtyley Nov 29, 2023

julienrf Nov 29, 2023

rtyley Nov 29, 2023

julienrf Nov 29, 2023

rtyley Nov 29, 2023

julienrf commented Nov 29, 2023

rtyley commented Nov 29, 2023

rtyley left a comment

Introduce task versionPolicyAssessCompatibility #184

Introduce task versionPolicyAssessCompatibility #184

Conversation

julienrf commented Nov 27, 2023 • edited

rtyley commented Nov 28, 2023

No changes (or adding a println to the middle of a method implementation)

Adding a new method

Removing an existing method

julienrf commented Nov 28, 2023 • edited

julienrf commented Nov 29, 2023 • edited

rtyley left a comment • edited

Choose a reason for hiding this comment

rtyley Nov 29, 2023

Choose a reason for hiding this comment

julienrf Nov 29, 2023

Choose a reason for hiding this comment

rtyley Nov 29, 2023

Choose a reason for hiding this comment

julienrf Nov 29, 2023

Choose a reason for hiding this comment

rtyley Nov 29, 2023

Choose a reason for hiding this comment

julienrf commented Nov 29, 2023

rtyley commented Nov 29, 2023

rtyley left a comment

Choose a reason for hiding this comment

julienrf commented Nov 27, 2023 •

edited

No changes (or adding a `println` to the middle of a method implementation)

julienrf commented Nov 28, 2023 •

edited

julienrf commented Nov 29, 2023 •

edited

rtyley left a comment •

edited