Open diareuse opened 9 months ago
I see how useful such a mode could be. However, there are a few concerns worth mentioning.
With a workflow where additions to the .api
file don't fail the validation and these changes have to be committed before a release, there are at least two scenarios when things could go wrong:
.api
was not updated before a release, BCV will no longer be capable of warning if some recently added declarations will be later removed (i.e., someone added a public class C
, it was released without .api
being updated, someone later removed class C
; since there was no such a class in a dump, BCV won't catch the removal).This means that developers who are willing to use incremental validation have to opt for it explicitly and should be informed about potential risks.
As a side note:
Therefore it poses a slight deviation from "Regular Workflow", but for a good reason. If the developer works on a gargantuan project, they should not be asked to study the project configuration as they are only responsible for making changes and not breaking compatibility. This is in turn a responsibility of the plugin to verify. In other words their time is better spent studying the current domain, instead of the project build harness.
It's no longer the plugin's responsibility as soon as validation requires some multistep workflow stretched in time and involving multiple cooperating agents to make it work correctly.
While it might seem like a simplification, incremental validation will probably make the overall release process more complex.
I agree with multiple, nearly all, points you've made, but let me elaborate anyway.
…some unintended ABI updates may leak into a release.
I agree and that's why I don't envision this being the default anytime soon. I feel like enabling this mode would need thorough consideration and understanding of the potential implications by the implementer. I'm not sure how to warn users sufficiently and if you have any ideas, I'm willing to include them into my PR.
That said I feel like the potential user is a sane person who knows what they are doing, therefore considering the implications based on the mode they have explicitly enabled is out of the question. In other words: I don't believe the user is stupid.
This means that developers who are willing to use incremental validation have to opt for it explicitly and should be informed about potential risks.
I'd cave into giving them suggestion on a good workflow and to advise that branch protections with sufficient checks and automation are absolute must when using this particular mode. But then again I want to believe most of them use this already.
I'm considering this a good workflow and would love to know your opinion about it:
It's no longer the plugin's responsibility…
I feel like we have a misunderstanding regarding the responsibilities here. From my point of view, in the current state the plugin is that it's responsible for failing on every single change, therefore it does not verify the compatibility, it verifies the identity. The compatibility responsibility is deferred to the developer which might or might not have the understanding of this particular plugin and how to make the correct decision about the API being compatible.
My intention is to shift this responsibility to the party which understands these changes and make a correct decision for them. We can discuss how to improve this decision-making process, but I would prefer to do this after user feedback due to obvious reasons.
…incremental validation will probably make the overall release process more complex.
I agree, but not in terms of the developer workflow, but the devops team. I'd love to trade in 1 person's afternoon for the team of 30 saving 15 minutes on every PR. Would you not?
In any case, these are great points and thanks for voicing them 👌
The compatibility responsibility is deferred to the developer which might or might not have the understanding of this particular plugin and how to make the correct decision about the API being compatible.
I guess you need to have an understanding of what makes an incompatible change to the public binary API (which is is general Kotlin/JVM knowledge), rather than of this particular plugin to make this judgement.
In any case, it would be good if this judgement can be automated by this plugin.
If we are worried about changing the workflow, what about just telling the user whether a change is compatible or breaking in the error message as a start?
However, it seems like judging what is binary compatible or not is not so clear-cut as you might think. Adding an abstract method to a class or interface is not strictly a breaking change, but it might causes unwanted runtime failures, so it might be useful to flag it anyway (which is currently done, but won't be with this PR).
The Problem
Most of the time the developer wants to know whether the changes done to the project are compatible with the previous version. This is not currently possible due to the hard fail during the
apiCheck
(check
) phase. It verifies that the API has not changed at all, which loads the developer with more chores and responsibilities, that could be inherently automated.Suggested solution
Introduce a mode in which compatibility validator actually only validates the binary compatibility (not identity) to the previous version. These changes almost exclusively include additions - new methods, new fields, new classes.
Since the compatibility validator already conveniently provides a diff of lines changed, new mode can be introduced to allow searching for removals and in only that case, hard fail the task.
It naturally allows for a workflow which could be similar to following:
Therefore it poses a slight deviation from "Regular Workflow", but for a good reason. If the developer works on a gargantuan project, they should not be asked to study the project configuration as they are only responsible for making changes and not breaking compatibility. This is in turn a responsibility of the plugin to verify. In other words their time is better spent studying the current domain, instead of the project build harness.
Regular Workflow therefore (speculation) proposes isolated instances of different kinds of build/verification instrumentation for each team or module, as the team would have to know instrumentation tasks to essentially not break compatibility if not done automatically. This is what I'd love to avoid.
I submitted a PR #186 which resolves this problem and allows for incrementally compatible checks only.
Obviously there's a plethora of options that could be introduced to the gradle extension to expand this feature to the moon. However to keep the scope for this new feature small, I opted for a simple flag which swaps the internal implementation of the
apiCheck
task.