v2.17.1 is a patch bump with a breaking change

[realtime-neil]

This is a breaking change: aa65fdd#diff-73eaf3fd93adb8d4747d040e3a2ef2eaL66

Will you bump the major version number? What is the workaround if you don't bump the major version number?

Originally posted by @realtime-neil in aa65fdd#commitcomment-31821515

[dorodnic]

Hi @realtime-neil

Thank you for raising this point.

The versioning scheme we have in use is somewhat different from the standard semantic versioning, with API changes triggering minor bump instead of major. In other words, we ensure that versions with same major.minor but different patch.build are safely interchangeable, without the need to recompile the application.

For the 2.17.1 release we haven't updated the list of API changes just yet, but AFAIK, the following PRs constitute change from 2.17:
#2830 - export processing blocks
#2829 - emitter ON/OFF option
#2823 - removal of get_removed_devices

For #2830, the changes are for now entirely "header only" and are not affected by the version number in rs.h. These are, however, candidates to be added to the official API and the different wrappers in future releases.

In case of #2829 - the firmware enabling this feature is not yet available outside Intel, so only internal users can be affected by the API change. After several delays this feature is currently scheduled to be supported in next firmware / LRS release.

In case of #2823 - we removed a function that always caused an exception. I don't think anyone will be affected by this change.

So while technically, this should have been 2.18, I think the changes are minor enough to keep it practically interchangeable with 2.17.0. This way you don't have to rebuild your application after sudo apt update && sudo apt upgrade. Hope this makes sense.

[realtime-neil]

@dorodnic , I've either misunderstood something crucial, or we both are using the same terminology to reference wildly different things.

Semver delineates interface changes along a "non-breaking" and "breaking" basis. The addition of a new function is an example of the former and must bump the minor (if not also the major). The deletion of an extant function (even one that "always caused an exception") is an example of the latter and must bump the major.

I now understand that you have elected to treat the major.minor tuple the way semver treats the major number; i.e., a breaking change to librealsense2-dev causes a major.minor bump. I can understand your reasons for doing so, even if I wouldn't choose it for myself.

Suffice it to say that Debian packages don't work that way. By publishing librealsense2-dev_2.17.1-0~realsense0.372 under the previous Debian package name, you have allowed a breaking change --- to the public interface --- to occur with a package upgrade. This is a Very Bad Thing, regardless of your expectations concerning how it will affect your end-users.

Do you intend for librealsense2-dev to comply with Debian policy? If so, how do you plan to rectify the situation?

[dorodnic]

It is not unreasonable to recall a newly published release and re-tag it under a different version, given good enough reasons.
Could you please clarify in what way the package violates the linked paragraph?
Also, be it a Very Bad Thing, is there any actual customer impact that I am missing?

[realtime-neil]

If there are development files associated with a shared library, the source package needs to generate a binary development package named libraryname-dev, or if you need to support multiple development versions at a time, librarynameapiversion-dev. Installing the development package must result in installation of all the development files necessary for compiling programs against that shared library.

-- https://www.debian.org/doc/debian-policy/ch-sharedlibs.html#development-files

As version that is backward-incompatible with the previous, librealsense2-dev_2.17.1-0~realsense0.372 must be considered an entirely different development version. This, in turn, necessitates the publication of "multiple development versions", each with a unique package name. As the quoted paragraph suggests (and convention dictates), the package name should contain the version string that broke the interface; e.g., librealsense2171-dev.

I can only speak for myself and my company: this breaking change broke our build. That the member function caused run-time exceptions is immaterial to the mandate that package upgrades should never break existing code. I'm having trouble finding wording within the Debian policy docs that spells this out --- perhaps, I think, because interface stability is implicitly assumed so widely. I'll keep looking.

[dorodnic]

Hi @realtime-neil

If the change broke your build we will do our best to rectify it. I need to do my homework and better understand why this happened. If you could share more information it would help a lot.

This is especially strange since the function in question is not part of shared lib's exported API, it exists in header-only, and is being compiled by the application, not librealsense.

My understanding until now was that publishing librealsense2-dev_2.18.0-0~realsense0.xxx with an API change would be Ok within our usage of the SONAME mechanics, and we didn't not receive any negative feedback in such cases so far, but I'm looping-in @ev-mp who designed librealsense Debian package to look into this.

[realtime-neil]

@dorodnic , as of this writing, here is the enumeration of your public headers:

$ apt-cache policy librealsense2-dev | head -3 && dpkg -L librealsense2-dev | grep include
librealsense2-dev:
  Installed: 2.17.1-0~realsense0.372
  Candidate: 2.17.1-0~realsense0.372    
/usr/include
/usr/include/librealsense2
/usr/include/librealsense2/rs_advanced_mode.h
/usr/include/librealsense2/rsutil.h
/usr/include/librealsense2/rs.h
/usr/include/librealsense2/hpp
/usr/include/librealsense2/hpp/rs_internal.hpp
/usr/include/librealsense2/hpp/rs_device.hpp
/usr/include/librealsense2/hpp/rs_processing.hpp
/usr/include/librealsense2/hpp/rs_frame.hpp
/usr/include/librealsense2/hpp/rs_export.hpp
/usr/include/librealsense2/hpp/rs_context.hpp
/usr/include/librealsense2/hpp/rs_pipeline.hpp
/usr/include/librealsense2/hpp/rs_sensor.hpp
/usr/include/librealsense2/hpp/rs_record_playback.hpp
/usr/include/librealsense2/hpp/rs_types.hpp
/usr/include/librealsense2/rs_advanced_mode.hpp
/usr/include/librealsense2/h
/usr/include/librealsense2/h/rs_processing.h
/usr/include/librealsense2/h/rs_record_playback.h
/usr/include/librealsense2/h/rs_config.h
/usr/include/librealsense2/h/rs_types.h
/usr/include/librealsense2/h/rs_frame.h
/usr/include/librealsense2/h/rs_internal.h
/usr/include/librealsense2/h/rs_device.h
/usr/include/librealsense2/h/rs_sensor.h
/usr/include/librealsense2/h/rs_context.h
/usr/include/librealsense2/h/rs_pipeline.h
/usr/include/librealsense2/h/rs_advanced_mode_command.h
/usr/include/librealsense2/h/rs_option.h
/usr/include/librealsense2/rs.hpp

If you see "private" headers in the aforementioned list, then I would ask that you review your packaging methodology.

How do you package librealsense2 and librealsense2-dev? Do you have what ROS and gbp call a "release repository"? Where is your debian directory being kept?

[realtime-neil]

@dorodnic @RealSense-Customer-Engineering @ev-mp : for what it's worth, my very nice co-workers removed the call to the probably-should-have-been-private-but-was-totally-public function get_removed_devices, so crisis averted and all that. How's the packaging review going?

[ev-mp]

@realtime-neil ,
Sorry for the delay and thanks for the inputs - we took the time to review the packages content against the Debian's policy for Shared libs :

• The policy provides guidance how to maintain the shared object versions with SONAME symlink. First, the policy defines its scope and elaborates:

This section deals only with public shared libraries: shared libraries that are placed in directories searched by the dynamic linker by default or which are intended to be linked against normally...

The policy describes how the changes in the shared lib ABI should be handled and provides an explicit definition of what constitute a change that requires SONAME update:

Every time the shared library ABI changes in a way that may break binaries linked against older versions of the shared library, the SONAME of the library and the corresponding name for the binary package containing the runtime shared library should change...

The key points are:

• The Debian versioning scheme applies to shared objects.
• The shared objects are defined as modules that are either dynamically loaded, or linked against.
  However, not less important what the policy lacks of:
• The policy does not provide guidance for header files or other non-binary artifacts that are not part of the shared object's exposed API (chapter 8.4), which is actually consistent with the policy's declared scope.

Back to the commit in question:

• The header file with the function removed is not part of the shared object SONAME binary interface, but an utility used in the python wrapper of the library.
  Hence modifying it does not constitute a breaking change to the shared object ABI. E.g the code linked against v2.17.1 SONAME (librealsense2.so) is not affected by this change.
  The module that is being affected is the python wrapper, but it is not subject to Ubuntu versioning convention (semver) and distributed with a separate python's pip deployment mechanism.

Given this breakdown we do not find this commit breaking the established Debian policy rules.

[realtime-neil]

Well done. Masterfully defended. Because the breaking change was the removal of a function with an inlined definition, you successfully isolated the breakage to the header that contained it. And, as you accurately point out, section 8.4 of the Debian policy is rather quiet on that point.

If that function definition had been in the library, then I might have won this argument. As it is, the letter (if not the spirit) of the policy supports you and not me.