Semantic versioning policy and dart 4.3

[scpeters]

I noticed that a tag was created for dart 4.3.0. When comparing v4.2.0 with v4.3.0, I notice that there are some API changes, changing header file and class names for example (Constraint -> ConstraintBase).

What is the policy about semantic versioning? Should this release be relabeled as dart 5.0 since it changes the API?

@j-rivero

[jslee02]

Basically, the policy is that API compatibility should be satisfied in the minor version changes. But I can say that it's not that restrict. I've made some API changes in minor version ups for the API that is not exposed to the end-users. If we bump the major version number for every (small) API changes, then it will goes up too fast I think.

I guess that the ideal solution would be enforcing the policy. For this version up, how can I keep the API compatibility with the header file and class name changing?

[scpeters]

In gazebo pull request 638, I wrote a script that would auto-generate header files with the old name that #include the new name and give a #warning about the header file name change (see issue_775_generator.bash).

We only install that on case-sensitive filesystems, since it's impossible on the non-case-sensitive ones. I'm not sure about mitigating changed class names.

[scpeters]

This is especially relevant since this breaks gazebo:

Linking CXX executable gzserver
physics/dart/libgazebo_physics_dart.so.5.0.0: undefined reference to `dart::simulation::World::step()'
physics/dart/libgazebo_physics_dart.so.5.0.0: undefined reference to `dart::dynamics::Joint::setName(std::string const&)'
physics/dart/libgazebo_physics_dart.so.5.0.0: undefined reference to `dart::constraint::ConstraintSolver::addConstraint(dart::constraint::Constraint*)'
physics/dart/libgazebo_physics_dart.so.5.0.0: undefined reference to `dart::constraint::ConstraintSolver::removeConstraint(dart::constraint::Constraint*)'
collect2: error: ld returned 1 exit status
make[3]: *** [gazebo/gzserver-5.0.0] Error 1
make[2]: *** [gazebo/CMakeFiles/gzserver.dir/all] Error 2
make[1]: *** [gazebo/CMakeFiles/gzserver.dir/rule] Error 2
make: *** [gzserver] Error 2

[scpeters]

Dart's cmake version config file uses SameMajorVersion, so if you break API from 4.2 to 4.3, then it will be much more difficult for downstream users to select a compatible version.

Keeping the major version number low may be important to you, but they also have a useful function for people using cmake find_package, so for that reason, I suggest bumping the major version to 5.

@j-rivero

[j-rivero]

There are a good bunch of project that breaks ABI on minors. As far as I know DART is one of them. Steve is right, we should patch the version config file to declare SameMinorVersion.

The option of bumping ABI only on major depends on every development team. Steve is right (as usual) when recommend to follow the general convention on semantic versioning but this is not mandatory. The key point is to implement the library soname and soversion according with the ABI policy and make the packaging right.

[scpeters]

Good point @j-rivero , we could switch to SameMinorVersion with the following if you don't want to increase to dart 5.0 right now:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 104a5e4..e1f97e1 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -423,7 +423,7 @@ set(DART_CORE_CONFIG_OUT ${CMAKE_BINARY_DIR}/cmake/DARTCoreConfig.cmake)
 message(STATUS ${DART_CORE_CONFIG_OUT})
 message(STATUS ${CMAKE_BINARY_DIR}/cmake/DARTConfigVersion.cmake)
 configure_file(${DART_CORE_CONFIG_IN} ${DART_CORE_CONFIG_OUT} @ONLY)
-write_basic_config_version_file(cmake/DARTCoreConfigVersion.cmake VERSION ${DART_VERSION} COMPATIBILITY SameMajorVersion)
+write_basic_config_version_file(cmake/DARTCoreConfigVersion.cmake VERSION ${DART_VERSION} COMPATIBILITY SameMinorVersion)
 install(FILES ${DART_CORE_CONFIG_OUT} ${CMAKE_BINARY_DIR}/cmake/DARTCoreConfigVersion.cmake DESTINATION share/dartcore)
 if(NOT BUILD_CORE_ONLY)
   set(DART_CONFIG_IN ${CMAKE_SOURCE_DIR}/cmake/DARTConfig.cmake.in)
@@ -431,7 +431,7 @@ if(NOT BUILD_CORE_ONLY)
   message(STATUS ${DART_CONFIG_OUT})
   message(STATUS ${CMAKE_BINARY_DIR}/cmake/DARTConfigVersion.cmake)
   configure_file(${DART_CONFIG_IN} ${DART_CONFIG_OUT} @ONLY)
-  write_basic_config_version_file(cmake/DARTConfigVersion.cmake VERSION ${DART_VERSION} COMPATIBILITY SameMajorVersion)
+  write_basic_config_version_file(cmake/DARTConfigVersion.cmake VERSION ${DART_VERSION} COMPATIBILITY SameMinorVersion)
   install(FILES ${DART_CONFIG_OUT} ${CMAKE_BINARY_DIR}/cmake/DARTConfigVersion.cmake DESTINATION share/dart)
 endif()

I would still advocate for bumping the major when API breaks to follow general conventions, but it's your decision.

[jslee02]

I read the general conventions carefully, and figured out that the major version ups should depend on the API compatibility rather than how many new features are added.

I think there are three options for now.

1. Staying DART 4.3 by switching to SameMinorVersion
2. Staying DART 4.3 by applying similar solution to what gazebo did
3. Bumping the major version to DART 5.0

Option 1 seems not following the general conventions so I don't prefer to go. If Constraint class name and its file name change are the only API changes then I'd like to go Option 2, otherwise Option 3.

[scpeters]

The gazebo pull request compensated for changing header file names, but not changing class names. It's probably possible, but we would have to figure out how to do it.

[mxgrey]

A quick and dirty way of dealing with a changed class name would be to make a class with the old name that inherits the new class, as well as inherits the new class's constructors (assuming C++11 compatibility). But this might not be permissible if there was a particular reason that the old name was replaced.

[jslee02]

I'm considering using typedef (or using which is introduced by C++11) like:

typedef ConstraintBase Constraint;
using Constraint = ConstraintBase;  // C++11

[mxgrey]

+1 to typedef

[mxgrey]

On a personal note, I have some changes that I plan on deploying within the next couple of weeks that would deprecate major sections of the API and add a lot of new API features. So far I don't think my changes will conflict with the existing API, but there will be a lot of deprecation warnings all over the place, which could be annoying.

The reason I bring this up is I'm not clear if it would be better to remove the deprecated parts of the API with the very next major version upgrade or if it would be better to leave the deprecated parts of the API for an entire major version to give the end-user some time to adjust. What would be the preferred approach, so I can plan ahead?

[jslee02]

I'd like to remove deprecated API with every next major version upgrade. Once I thought that we should give the end-user some time to adjust until like the next few minor version upgrades, but it may too hard to do if the change is huge.

One of my next plan for DART 5.0 is to reorganize the namespaces with tree structured hierarchy. For example, the dependency of dynamics namespace will be removed from collision namespace, and constraint namespace will be merged to dynamics. This will make huge change in the files and directories so it will not possible to keep the deprecated APIs.

I still want to know what would be the preferred approach in general too.

[scpeters]

The deprecation warnings are only helpful if users upgrade to the deprecated version. If they skip a version, they may miss the deprecation warnings completely and just have broken code.

If you want to deprecate for one cycle and then remove, that's fine, as long as the cycle is long enough for people to migrate their code (a couple of months minimum?).

[jslee02]

LCPSolver.h and lcpsolver.h conflicts as well.

[jslee02]

@scpeters How can I replicate the errors you got?

Linking CXX executable gzserver
physics/dart/libgazebo_physics_dart.so.5.0.0: undefined reference to `dart::simulation::World::step()'
physics/dart/libgazebo_physics_dart.so.5.0.0: undefined reference to `dart::dynamics::Joint::setName(std::string const&)'
physics/dart/libgazebo_physics_dart.so.5.0.0: undefined reference to `dart::constraint::ConstraintSolver::addConstraint(dart::constraint::Constraint*)'
physics/dart/libgazebo_physics_dart.so.5.0.0: undefined reference to `dart::constraint::ConstraintSolver::removeConstraint(dart::constraint::Constraint*)'
collect2: error: ld returned 1 exit status
make[3]: *** [gazebo/gzserver-5.0.0] Error 1
make[2]: *** [gazebo/CMakeFiles/gzserver.dir/all] Error 2
make[1]: *** [gazebo/CMakeFiles/gzserver.dir/rule] Error 2
make: *** [gzserver] Error 2

[scpeters]

I was building gazebo with branch gazebo5 against the dart master branch. I'm now testing the branch from #309.

[jslee02]

Fixed by #309. Thanks for the valuable comments.

[scpeters]

Thanks for keeping API compatibility. Also, I just update the homebrew formula to 4.3.1 in dartsim/homebrew-dart@6102d8f

[jslee02]

I fixed installation bug (didn't install utils/urdf and utils/sdf) so had to bump up version to 4.3.2 with the patch. The homebrew formula is also updated.