Repository navigation
Breaking API check never fails #304
Description
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?
Activity
mosuem commentedon Oct 9, 2024
It seems like you found the fix already, adding
breakingto thefail_onlist in the settings.jakemac53 commentedon Oct 9, 2024
I did this but it didn't actually make the job fail still
mosuem commentedon Oct 9, 2024
It does pass now, as the version is correct. Can you point to the invocation which incorrectly passed?
jakemac53 commentedon Oct 9, 2024
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 commentedon Oct 9, 2024
Why should it be failing? The comment says it needs a minor version bump, which was made. What am I missing?
jakemac53 commentedon Oct 9, 2024
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 commentedon Oct 9, 2024
@devmil - Widening the return type seems to be a minor breaking change at the moment - should it be a major breaking change instead?
devmil commentedon Oct 9, 2024
I agree. I created an issue to look into this
devmil commentedon Oct 10, 2024
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 commentedon Oct 10, 2024
Hm. But the
versionpart of the report seems to indicate that the version should only be bumped from4.1.0to4.2.0, or am I overlooking something?mosuem commentedon Oct 10, 2024
Also, I thought
"type": "minor"indicates that this warrants only a minor version change, or what does it mean?devmil commentedon Oct 10, 2024
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 commentedon Oct 10, 2024
Sounds good - and the
versionkey of the report should then also suggest5.0.0, correct?devmil commentedon Oct 10, 2024
['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 commentedon Oct 10, 2024
Yes, the
neededfield is the one being parsed.devmil commentedon Oct 10, 2024
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 commentedon Oct 10, 2024
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 commentedon Oct 10, 2024
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 commentedon Oct 10, 2024
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
-wipversion, and even the number associated with that will not have been published yet).