About API breaking changes

Development
1

On a ParaView MR, we started having a discussion which should be discussed here.
https://gitlab.kitware.com/paraview/paraview/merge_requests/3396

ParaView uses VTK as a submodule and usually the developer that update the VTK submodule is responsible for fixing any issue that may arise related to the submodule update.

It means that sometimes, based only on luck, this developer as to fix ParaView so it builds and works correctly with new changes in VTK that have nothing to do with the reason he is currently updating the VTK submodules.

In my opinion, it is completely fine to do that when the changes needed are a few baseline updates or small warning fixes that can be fixed in a matter of minutes.

It is not fine when it requires to spend multiple hours, especially if a deadline is coming or if the developer has no expertise at all in the related code. It is hard to draw the line, though, so here are some ideas to improve that :

  • Developers working on VTK and software dependent of VTK should be diligent in testing their VTK branch with these dependent software before merging and assume the responsibility of correcting any issue that may arise, even if they do not update VTK themselves. This is in general already done imo, but could be formalized.

  • API breaking changes or profound non-retro-compatible changes could be announced, a few days before they are merged, with ideally an idea on how to fix issues and build failures in the software that depend on ParaView. These could then be compiled into a porting guide provided with VTK releases.

  • VTK buildbots could be dedicated to build VTK-dependent software that we develop at Kitware (ParaView, Slicer, CMB, Tomviz…), so VTK only developer could inform ahead of time that someone, on these projects, will need to take care of correcting the build issues.

What are your takes on that ?

2

While these are noble goals, my feeling is that these are impractical. Having the onus on the VTK developer to ensure no dependent projects are broken is just not feasible. Imagine the QVTKWidget changes you were working on a while ago. Now imagine doing those changes tracking down dashboard failures on all dependent projects, CMB, TomViz, Slicer etc. etc. !

Having ability for buildbot to trigger builds on dependent project has indeed been an idea we’ve discussed and deemed useful. It’s awaiting developers to take on the implementation.

3

I would expect that VTK developers do a lot of testing and planning to avoid major regressions in releases. However, I don’t think we should try to control more tightly what happens in the VTK master, as any extra requirements would just put more burden on VTK developers.

In Slicer, we use our own VTK fork where we can selectively backport those VTK fixes that we need, without worrying that we bring recent unrelated VTK changes that could cause trouble. When the time is right (there is a new VTK release, we don’t have any pressing deadlines, and/or we want to take advantage of larger developments/design changes in VTK) we synchronize our VTK fork to the master.

4

I absolutely agree and this was never my suggestion. I merely suggested that developper that already work on VTK and VTK dependant software (like VTK and ParaView for me) test the VTK dependant software (only ParaView for me) locally before merge as they already have the expertise to do so.

Great !

Regression should not happen in master if testing is done right, but in any case, this is about API changes. My Second suggestion would improve the release update process by providing a compiled document of all the necessary changes in dependent software. We more or less have a similar mechanism in ParaView where we document important changes in .md files. Should’nt we have something like that in VTK in order to help VTK software dev to update their VTK internally ?

5

This problem rears its head every now and then and is particularly frustrating for ParaView developers since ParaView tracks VTK so closely. Other projects that depend on VTK updating less frequently, and when they do there is an expected amount of pain to handle API changes. ParaView developers hit that pain unexpectedly, and that is a source of major frustration. We recognize the problem, propose the same ideas, and then the frustration passes and we have other things to do and those ideas don’t get implemented (e.g., testing applications against nightly VTK, improving API change docs in VTK, etc.).

So far, IMO the best approach to avoiding that pain is to avoid API-breaking changes whenever possible. That also has the benefit of not irritating other projects that depend on VTK. I won’t go so far as to say no breaking API changes should be allowed, and some are necessary surgery on a 25-year-old patient, but erring on the side of compatibility seems like a good default. I think it is a judgement call when these changes are up for review in a merge request where you weight the cost vs. benefit of the breaking change before deciding to accept it.

We should, somebody just has to set it up. For 7.1, there is documentation on changes, but it isn’t linked in any convenient way from the main VTK documentation page.

Unfortunately, documentation doesn’t solve the problem with updating certain ParaView dependencies that depend on VTK. That has more to do with the cumbersome (but necessary) process of updating certain third party dependencies in ParaView than it does with VTK making a relatively minor API change.

6

In 2006 I wrote a draft API Change Policy for ITK. The Background section, repeated here, discusses many of the issues raised here. I argued that the time and money our customers invest in our open-source software is not insignificant. We don’t know the extent of our customer base. It’s ironic, that Paraview, a customer of VTK, is being affected by API changes in VTK. My document was not accepted, and many argued that customers should accept progress or stick with a version of the code that does not break the API.

One of the major criticisms of open-source software is that new revisions are not compatible with old revisions. Breaking compatibility impedes the acceptance and utility of open-source software. On the other hand, strict backward compatibility policies can impede innovation in software. The tension between these two viewpoints is not easily resolved.

As projects mature and the customer base grows, backward compatibility becomes more important. Commercial hardware and software products call this customer base, the installed base. Commercial products usually have a known customer base consisting of those who have purchased or licensed the software. Also, commercial systems seldom expose internal AP I’s. Open source projects rarely know the identities of their customers.
And, since the source is open, customers have access to all public and protected classes, methods and data in the code. For open-source software, it is almost impossible to determine how the customer base is using the software.

When a project hits a certain point in its life cycle, the unpleasant issue of
backward compatibility begins to rear its ugly head. All of a sudden the changes introduced in a new release of the software have a dark side to them; they hold hidden possibilities that will break something one of your users depends on.
This is true in both open and closed source projects, but in the open-source
world it seems that the community has spent less time worrying about it than in the closed source world. “Preserving Backward Compatibility,” Garrett Rooney.

The Dark Side of Extreme Programming: The nightly test/build was so effective in empowering programmers to make changes, that API changes occurred too frequently without the necessary buy-in from the user community.
From “Inside Insight,” Bill Lorensen

Some argue that open-source software should be used at your own risk. But even using open-source software requires an investment in time, energy, and funds. Also, the reputation of the development community is at risk.
…consider your user base. If you have only a dozen highly technical users,
jumping through hoops to maintain backward compatibility may be more trouble than it’s worth.
On the other hand, if you have hundreds or thousands of nontechnical users who cannot deal with manual upgrade steps, you need to spend a lot of time worrying about those kinds of issues. Otherwise, the first time you break compatibility, you’ll quickly burn through all the goodwill you built up with your users by providing them with a useful program. It’s remarkable how easily people forget the good things from a program as soon as they encounter the first real problem.
From “Preserving Backward Compatibility,” Garrett Rooney.

These investments are made by customers that include developers, users, and sponsors.

Here is the original proposal.
APIChangePolicy.pdf (44.7 KB)

7

A good document, but I have reservations about certain points.

those changes should never cause user code to fail to compile

Too extreme imo. Deprecated code can be removed at some point.

At run-time, deprecated API’s report how to change code from the old API to the new API.

It may refer to documentation instead.

8

I think the simple way to solve that without significant effort is to try to deprecate changed apis, comment in the header how to migrate to the new api, and remove this deprecated api after the next release.
It’s not always possible to keep the functionality, but we should at least try to not break the build of VTK customer when updating a minor version without previous deprecation warning.
That being said, having a builbot testing at least the build of ParaView would be awesome :slight_smile:

1 Like
9

My thoughts at that time (13 years ago), were a bit extreme. In the heavily templated ITK, single compile error could go on for pages… Today’s compilers often give good suggestions on how to fix the code.

10

Den ons 17 juli 2019 09:56Mathieu Westphal (Kitware) via VTK noreply@discourse.vtk.org skrev:

A good document, but I have reservations about certain points.

those changes should never cause user code to fail to compile

Too extreme imo. Deprecated code can be removed at some point.

As a user of VTK I think it would be nice if VTK followed an ABI/API compat and deprecation policy similar to Qt’s. That is, maintain compat across minor versions, but allow breaks across major ones, and have deprecation macros so that use of deprecated APIs show up as warnings for at least one minor version before being dropped in a major one.

It is of course up to you folks what kind of commitment you’re willing to make, and I understand if this would be a too big ask.

We’re one of those Kitware-external users who, unlike e.g. ParaView, do not track VTK master. But we try to follow each minor release. It would be nice if, like with Qt, we could be guaranteed an essentially painless upgrade when moving between minor versions.

Cheers,

Elvis

11

We would need to fix VTK_LEGACY_REMOVE to work as a versioned check rather than be a global on/off switch. This would allow deprecations to be marked with VTK_LEGACY_API(8, 90, 0, 20190723) where ParaView could then set VTK_COMPAT_API=VTK_COMPAT_VERSION(8, 90, 0, 20190722) (or something) to say it is OK with API removals up until the 22nd and hides anything with a removal older than that and warns for anything deprecated since then.

3 Likes
12

@ben.boeckel : That would indeed be a very nice to implement that. I think not only ParaView would benefit but other VTK dependant software as well.

In the meantime, actually deprecating public API instead of removing it altogether would already be a nice touch.

13

I’m 100% OK with such a change. I do not have time to implement, test, and document, but I can help with the review.

As for the API guidelines, I suggest notifying those who do this via the MRs that break it and tell/ask them not to do it anymore. I don’t know how many read this board.

14

Issue created :
https://gitlab.kitware.com/vtk/vtk/issues/17649

I’ve also created an issue for the buildbot as well :
https://gitlab.kitware.com/vtk/vtk/issues/17650

And use actual deprecation instead !

15

I’m trying deprecate a method cleanly right now. I’m only trying to add a another argument to a virtual method.

It is doable but definitely not easy nor trivial.

Also, I couldn’t find a way to inform a potential VTK users that he is inheriting from a deprecated method at compilation time.

16

They’ll get -Woverload-virtual warnings because their override is shadowing the other overload they’re not using.

17

with VTK_LEGACY_REMOVE=ON indeed, but not with VTK_LEGACY_REMOVE=OFF.

18

They’ll actually get errors in that case. Their override on the method will say “you’re not overriding anything”. And they’ll get the -Woverload-virtual warning.

19

Not sure to follow. With VTK_LEGACY_REMOVE=OFF, there should be no errors and the results should be the same. There should be warnings though.

If they are not overriding, then results can’t be the same.

If you want to take a look into the specifics of my implementation, it’s here https://gitlab.kitware.com/vtk/vtk/merge_requests/5915, especially 1a56e913efbf778c068eb3420ae8b82e8fb5a5c6

20

FYI : my “non-breaking-api” deprecation change have been merged. It was more complex then expected and may be of interest of people trying to achieve the same thing.
https://gitlab.kitware.com/vtk/vtk/commit/09e99a46df7ffb761abf0d7c5688ec70b476d4b1

For some explanation of the logic, see the discussions there :
https://gitlab.kitware.com/vtk/vtk/merge_requests/5915