Breaking API check never fails

[jakemac53]

I am trying to add the breaking API check here dart-lang/build#3760.

While in the comment it does notice the breaking change, and the job also sees the breaking change, it still passes, and also in the comment it is listed with a green checkmark.

I should be forced to do a breaking release?

[mosuem]

It seems like you found the fix already, adding breaking to the fail_on list in the settings.

[jakemac53]

It seems like you found the fix already, adding breaking to the fail_on list in the settings.

I did this but it didn't actually make the job fail still

[mosuem]

It does pass now, as the version is correct. Can you point to the invocation which incorrectly passed?

[jakemac53]

https://github.com/dart-lang/build/actions/runs/11150849769/job/30992945328?pr=3760 is the job which passes, but should be failing. It should be requiring a breaking release (5.0.0) but I only did a feature release.

[mosuem]

Why should it be failing? The comment says it needs a minor version bump, which was made. What am I missing?

[jakemac53]

Oh interesting, it was definitely a breaking change.

I widened the return type of a function, so users of that function would be broken.

So, I guess the issue is actually that widening return types should be considered breaking and cause a major version.

[mosuem]

@devmil - Widening the return type seems to be a minor breaking change at the moment - should it be a major breaking change instead?

[devmil]

@devmil - Widening the return type seems to be a minor breaking change at the moment - should it be a major breaking change instead?

I agree. I created an issue to look into this

[devmil]

@devmil - Widening the return type seems to be a minor breaking change at the moment - should it be a major breaking change instead?

Widening the type already should lead to a breaking change and looking into the CI run (https://github.com/dart-lang/build/actions/runs/11150849769/job/30992945328?pr=3760) it looks like dart_apitool detected that change as breaking?

[mosuem]

Hm. But the version part of the report seems to indicate that the version should only be bumped from 4.1.0 to 4.2.0, or am I overlooking something?

[mosuem]

Also, I thought "type": "minor" indicates that this warrants only a minor version change, or what does it mean?

[devmil]

Also, I thought "type": "minor" indicates that this warrants only a minor version change, or what does it mean?

Ah, I see. The confusion comes probably from the fact that the "type" in the JSON output is only relevant if breaking == "false" and tells if a minor version increase should happen.
If the change is breaking then a major bump has to happen and the "type" field gets irrelevant.

I will change that so that type contains "major" in that situation which would make much more sense in my opinion

[mosuem]

Sounds good - and the version key of the report should then also suggest 5.0.0, correct?

[devmil]

Sounds good - and the version key of the report should then also suggest 5.0.0, correct?

['version']['needed'] should already contain that information today

Edit: Looking at the generated comment it seems that 4.2 was suggested (as you said and assuming that the needed column is filled with the needed field). Will investigate further

[mosuem]

Yes, the needed field is the one being parsed.

[devmil]

Yes, the needed field is the one being parsed.

So, the problem is that the old version is a pre-release version. In that case there is no major bump requirement for breaking changes (dart_apitool only checks if the new version is equal or higher)
This is based on the assumption that the changes are always tested against the last released version. Not sure if the use case here might be different. If so then we might want to add an option to change that behavior

[mosuem]

Hm, that sounds like the API tool it is working correctly after all.

The health PR check currently tests against the repository checkout, it's an interesting question if it should test against the latest released version to check if a major version bump is required instead. That would require downloading and unzipping the version from pub.dev.

@jakemac53 any opinion for your workflow in the build repo?

[devmil]

Hm, that sounds like the API tool it is working correctly after all.

The health PR check currently tests against the repository checkout, it's an interesting question if it should test against the latest released version to check if a major version bump is required instead. That would require downloading and unzipping the version from pub.dev.

@jakemac53 any opinion for your workflow in the build repo?

dart_apitool can do that already if the package is on pub.dev so you would just specify the "pub://package/version" Uri as the old package parameter

[jakemac53]

The workflow we have been pushing for all dart-lang packages is that the first commit after a release always increases the version and adds a -wip. The repo no longer matches the state of the release and so this indicates that we are now working on the next release, and what the current planned version for that release is.

So we should be comparing against the last released version then, yes, and not using whatever the version in the pubspec was at the prior commit (which will usually be some -wip version, and even the number associated with that will not have been published yet).