Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatibility reports generation script #2410

Merged

Conversation

SergioRAgostinho
Copy link
Member

@SergioRAgostinho SergioRAgostinho commented Sep 1, 2018

A new useful script to generate the ABI compatibility reports scripts in a single go 💯

One more step in speeding up releases. I'm crunching the reports now and I'll upload them later.

@SergioRAgostinho
Copy link
Member Author

Here they are.
abi_reports.zip

@SergioRAgostinho SergioRAgostinho added this to the pcl-1.9.0 milestone Sep 1, 2018
@SergioRAgostinho
Copy link
Member Author

SergioRAgostinho commented Sep 1, 2018

Comment Reports

Some of them are spot on. I think this version of the tool still has some minor problems.

Common

  • We added an number of const qualifiers to parse.cpp functions.

Features

  • We removed a member negative_ from StatisticalOutlierRemoval<PCLPointCloud2>.
  • Weirdly enough, it's also reporting changes in SAC headers for a number of symbols.

IO

  • Removed members from HDLGrabber.
  • Changed types on some methods from HDLGrabber.

Octree

  • We renamed some iterator names. I think we typedefed things. It appears as low severity.
  • We modified the type of Iterator::depth_

Recognition

  • Reporting broken symbols from SAC and Registration.

Registration

  • We added some attributes to Registration and they won't be initialized unless people recompile.

Sample Consensus

  • We added const qualifier on a number of methods.

Segmentation

  • It complains with getPointCloudDifference for having removed the parameters although in theory we deprecated it. This one is strange. Edit: I think abi-compliance-checker does not handle very well header defined functions.

Surface

  • The MovingLeastSquares class suffered profound changes.
  • MarchingCubes had some members removed.

Visualization

  • We removed the PCLVisualizer::addCoordinateSystem() method apparently, although we actually didn't.

@SergioRAgostinho
Copy link
Member Author

abi-reports.tar.gz

@SergioRAgostinho
Copy link
Member Author

compat_reports.zip

Latest version of the compatibility reports. I think this script is good to go for now. It is missing proper error checking and abortion routines when things fail but we can tweak those later.

@taketwo
Copy link
Member

taketwo commented Sep 29, 2018

The script looks good and we don't have any bash coding standards anyway. However I haven't tried to use it myself. Do you think it's needed? Or OK to merge as is?

@SergioRAgostinho
Copy link
Member Author

Do you think it's needed? Or OK to merge as is?

Well… to be fully honest, the first time you run things and don’t have all the required tools things can get ver criptic until you realize what is actually missing. It’s kinda half-baked, so probably not gonna be a good idea to merge it if we expect our users to use it.

The challenge now is recreating a bare environment without any of these tools. I have to figure out if chroot is enough to solve this problem or if something more advanced is required.

@taketwo
Copy link
Member

taketwo commented Sep 30, 2018

I've tried this on my not-so-bare Ubuntu 16.04 environment and the only packages that I was missing are abi-dumper and abi-compliance-checker.

@SergioRAgostinho SergioRAgostinho changed the title Compatiblity reports generation script Compatibility reports generation script Oct 1, 2018
@taketwo
Copy link
Member

taketwo commented Oct 6, 2018

Add something like this on top?

if ! hash abi-dumper2 2>/dev/null || ! hash abi-compliance-checker 2>/dev/null; then
  echo "This script requires abi-dumper and abi-compliance-checker"
  echo "On Ubuntu: apt-get install abi-dumper abi-compliance-checker"
  exit 1
fi

@jspricke
Copy link
Member

jspricke commented Oct 6, 2018

Just for reference, PCL ABI is tracked here as well: https://abi-laboratory.pro/index.php?view=timeline&l=pcl

@taketwo
Copy link
Member

taketwo commented Oct 7, 2018

Yeah, we have this link in our README actually. However it's not up to date, the latest status is reported for some random point in time in May.

@SergioRAgostinho
Copy link
Member Author

Latest reports

compat_reports.zip

@SergioRAgostinho
Copy link
Member Author

Everything seems to be similar in this latest version. This one is also good to merge.

@taketwo
Copy link
Member

taketwo commented Oct 12, 2018

Sorry that I bring this up now, what about rename generate_reports.sh -> generate_abi_reports.sh? And probably squash everything into a single commit?

@taketwo taketwo merged commit 5d670f5 into PointCloudLibrary:master Oct 12, 2018
@SergioRAgostinho SergioRAgostinho deleted the abi-compatibility branch October 12, 2018 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants