PCL 1.8.0 release

[taketwo]

After more than one year and over 600 commits since 1.7.2, I think it is time to consider releasing a new version. Below is a tentative checklist for the release preparation:

• check for API breakage
• decide (and clarify) which modules to release (related: Modules included in releases #1351)
• merge Real Sense grabber for Windows
• deprecate old Intel perceptual computing grabber
• finish SAC refactoring (Make SampleConsensusModel::getSampleSize() a virtual function #987, getSampleSize issue #987 #1344, SAC Models Refactoring: isModelValid() #1075)
• prepare a changelog (Update changelog for 1.8.0 release #1384)
• update LCCP interface and merge new CPC algorithm implementation (New interface for LCCP #1389, Implemented Constrained Planar Cuts algorithm (CVPR 2015) #1278)

There are two interesting work-in-progress items (default alpha values #1141, optimizations #1361), however I think it is hard to foresee all effects of applying them. The unit test coverage is far from 100%, so we can not be entirely sure that we don't break something. Therefore, I would avoid merging them shortly before the release.

Another issue that is worth attention is #1275, however I don't think that it should hold off the release. Once we have a solution, we can roll out 1.8.1.

[taketwo]

I am very close to completing the changelog (only two months worth of merged pull requests left). Any volunteers for the first item? Any conclusive thoughts about the second?

[SergioRAgostinho]

I can provide some assistance if needed.

1- You do you check API breakage? Try to compile and run 1.7.2 tests on the current HEAD?
2- I already expressed my opinion on that one.

[taketwo]

@jspricke As a seasoned maintainer, could you please explain the procedure for such a check?

[jspricke]

Good question, we never did it before but it would be great to document API changes in the release notes.
This link has a number of options:
http://stackoverflow.com/questions/1969916/static-analysis-tool-to-detect-abi-breaks-in-c/4747235#4747235

[SergioRAgostinho]

👍 I'll give a try over the weekend.

[SergioRAgostinho]

Status update:
Given the options, I went with ABI Compliance Checker, which seemed to be the only one I could "quickly" run on OS X.

• How to invoke it
• How to build the XML descritor with examples

My current approach is the create and XML for each module which generates a shared lib, starting with pcl_common. Here's an example of the xml descriptors I'm using in case you want to replicate.

<version>
    1.7.2
</version>

<headers>
    /opt/pcl/install_1_7_2/include/pcl-1.7/pcl/common/
</headers>

<libs>
    /opt/pcl/install_1_7_2/lib/libpcl_common.1.7.2.dylib
</libs>

<include_paths>
    /usr/local/include/eigen3/
    /opt/pcl/install_1_7_2/include/pcl-1.7/
</include_paths>

<version>
    1.8.0
</version>

<headers>
    /opt/pcl/install_1_8_0/include/pcl-1.8/pcl/common/
</headers>

<libs>
    /opt/pcl/install_1_8_0/lib/libpcl_common.1.8.0.dylib
</libs>

<include_paths>
    /usr/local/include/eigen3/
    /opt/pcl/install_1_8_0/include/pcl-1.8/
</include_paths>

And here's a first look into the only report I was able to generate, ignore the ABI report for now.

Small comments (concerns while traversing this uncharted territory experience):

• I won't be able to test pcl_visualization, because it's throwing compilation errors.
• I have the impression that I need to use the same compiler (gcc which is the only one supported) for building pcl and running the compliance checker tool. (I'll need to rebuild everything again)
• ABI checking requires compiling with no optimisations, generating and ABI dump and then passing that as a descriptor.

[taketwo]

Now I recall I used the same tool to check ABI compatibility between 1.7.1 and 1.7.2. For reference, here is a report from last time.

[SergioRAgostinho]

@taketwo Can you share the XML descriptors you used for all the modules. It would save me some time :)

[taketwo]

I found the report in my mail. I'll need to check another laptop to see if the descriptors are still stored somewhere.

As a side note, I was recently browsing OpenCV cmake scripts and came across a generator for ABI compliance checker: OpenCVGenABI.cmake. We may consider borrowing this.

[taketwo]

Checked. Unfortunately, I don't have these config files anymore.

[SergioRAgostinho]

So here's a bundle with xml descriptors, abi dumps, compatibility reports and logs. For the time being, I created the xml descriptors manually. This was done on Linux.

General comments:

• Most of the modules are incompatible 😢
• In average, There are situations where the source incompatibility rate (%) is always bigger that the binary. "Wait! What!? How is that even possible?" (see below).
• The Source compatibility report is missclassifying situations like the one below, reporting the symbol as missing/added. Fortunately enough, this appears to not break affect binary compatibility. So the idea is might be to focus the attention on the binary reports only first and then double check the source report.

int foo();  //old
int foo(int bar = 0); //new

• Modules which depend on other modules, "inherit" their incompatibilities (expected).

So... a small example of things that were broken...

pcl_common:

• pcl::Exception [ 1.7.2 , 1.8.0 ] - Public member's types were changed and some members were even removed. The class no longer has the same size, so all inherited types are now binary incompatible.

[jspricke]

Note that we are ok with ABI breakage (the minor version is part of the Soname).
Breakages in the API could be a problem, but as long as we document them, they should be ok as well.

[taketwo]

So what's the conclusion here? I'm done with the changelog, so this is the last item preventing release.

[jspricke]

I had a quick look through the reports (thanks @SergioRAgostinho for creating them!).
I think we are basically fine. Some where we need to check:
#1156, #1119, #1294, #965.

What I did:
Load all html from the .xz, switch to "Source Compatibility", start looking at "Removed Symbols".
Note that a lot of them are actually duplicates.

[taketwo]

The first one should not break user code; the rest could, but what can we do about it?

[SergioRAgostinho]

I think the last one, should also be source compatible. The method was modified to be templated, so the compiler should still be able to infer the type even if not specified explicitly.

Good question, we never did it before but it would be great to document API changes in the release notes.

Giving some proper highlighting ⚠️

[taketwo]

We may add such signs in the changelog for each item that modified the API. I'll finalize the PR with log tonight and we'll merge it. Would you care then to send another one which adds signs?

[SergioRAgostinho]

@jspricke Are there only those 4 issues? You did start the sentence saying "Some where..." :)

@taketwo In case you want to add it yourself, the sign is :warning:, otherwise I'll make a PR on your PR.

Nevertheless, I think we should detail the API changes on the release notes.

[jspricke]

It was rather late, so I'm not sure I got everything (maybe doing a diff would help, as there are really a lot of duplicates).
Also, I guess we should document deprecated stuff, like this: git log pcl-1.7.2..master -Sdeprecate.

[SergioRAgostinho]

Summing up what I'm getting in terms of deprecations:

• [#941] Supervoxel coloring update
• [#1071] SAC Models Refactoring: getClassName()
• [#1115] Change default behavior of SupervoxelClustering
• [#1395] Remove PXCGrabber
• [#1396] Store required sample size inside SampleConsensusModel classes

Edit: Added #941.

[taketwo]

Thank you. We will put this list in the release description here. Actually, I would propose so change the description content compared with the previous releases. Keep only the list of most important new features and depreciations, and refer to CHANGES.md for the complete change list. Additionally, as discussed we may mark individual items in the list with ⚠️ to highlight pull requests that changed API.

[SergioRAgostinho]

I prefer that approach as well. I've added one more item to the deprecation list.

[SergioRAgostinho]

Hey guys, how are we on this?

[VictorLamoine]

I guess... work in progress 😄

Do we agree on this list of published modules? It's the actual default list!

BUILD_2d                         ON
BUILD_CUDA                       OFF
BUILD_GPU                        OFF
BUILD_apps                       OFF
BUILD_common                     ON
BUILD_examples                   OFF
BUILD_features                   ON
BUILD_filters                    ON
BUILD_geometry                   ON
BUILD_global_tests               OFF
BUILD_io                         ON
BUILD_kdtree                     ON
BUILD_keypoints                  ON
BUILD_ml                         ON
BUILD_octree                     ON
BUILD_outofcore                  ON
BUILD_people                     ON
BUILD_recognition                ON
BUILD_registration               ON
BUILD_sample_consensus           ON
BUILD_search                     ON
BUILD_segmentation               ON
BUILD_simulation                 OFF
BUILD_stereo                     ON
BUILD_surface                    ON
BUILD_surface_on_nurbs           OFF
BUILD_tools                      ON
BUILD_tracking                   ON
BUILD_visualization              ON

[SergioRAgostinho]

I have the impression that was in fact the final opinion. Just keep it as it is.

[taketwo]

So... we need to tag RC and later release. There is a certain sequence of actions involving creating new tags and merging master into them. I'm a bit fuzzy here because Jochen always did it himself. @jspricke would you mind to take care of this? I'll do announcements and later GitHub release.

[jspricke]

Actually there is not much behind it, just tag it (as I just did) ;).

The interesting thing is probably how to do further release candidates. I don't like having an extra branch in the origin repo for it, as it would clutter everyones clone. So I usually have my local RC branch and just push the tags of it.

[taketwo]

What about the commits that live in release "branches", but are not a part of the master? git log master..pcl-1.7.2 --no-merges gives quite a few. Most of them are commits that delete stuff that is not supposed to be released. But there are also ROS-related commits. Shouldn't we have them in the new release as well?

[jspricke]

As far as I can see, all ROS related commits are in master as well.

[taketwo]

You are right, all these ROS commits are included in the master, but with different hashes. So the effective difference between 1.7.2 and master is:

• removal of 2d, cuda, gpu, ml, simulation, stereo modules;
• removal of Brisk keypoints/features (de3906b);
• removal of GRSD descriptor (f98372b).

I'd vote to leave out cuda and gpu again. No supported, not maintained, mostly obsolete. No opinion on Brisk. For GRSD descriptor we had a problem with hitting the limit of symbols on MSVC if I remember correctly. Maybe an easier and more permanent solution will be to just disable pre-compilation of this class?

[jspricke]

We removed most of the modules because Radu though they where not ready, but he is ok with it now. I would vote with leaving cuda and gpu in but disabled, as it makes maintenance and new releases much easier.

I'm still not sure with Brisk because of being SSE4 only, but I would leave it in this time.

I have the impression that we fixed the MSVC problem somehow in master, maybe someone can give it a try?

[taketwo]

I have the impression that we fixed the MSVC problem somehow in master, maybe someone can give it a try?

You are right, we fixed it with #897. Also, @UnaNancyOwen mentioned in #1530 that he succeeded building the release candidate on Windows.

There are two small pending pull requests for SSE support (#1523, #1525). What about merging them and then tagging the release?

[taketwo]

I've updated CHANGES.md one last time (and pushed directly to master). I think we are good to go with the release. (But we'll need to change the release date inside CHANGES.md file when we are sure about it).
@jspricke do you want another RC, or would you tag release already?

[jspricke]

Thanks @taketwo! I've just tagged an other RC and send a mail to the developers list for feedback. If there is no serious problem coming up, I would propose to release in the next days.

[ahundt]

@jspricke There is a problem with cuda compilation on OS X as noted in the issue #1563 I created

[ahundt]

I've updated to RC2 and encountered the same issues as noted in #1563. Where is the appropriate place to bring this if I want the issue fixed before release?

[UnaNancyOwen]

PCL 1.8.0 have passed an about 2 months from the RC2 released.
Do you have plan to release PCL 1.8.0 RC3?

[hwdong]

but not work on windows10.