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

Implement the abichecker run on devel builds #681

Merged
merged 77 commits into from
Dec 16, 2019

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Oct 2, 2019

The PR tries to implement the ABI checking for ROS2 packages (issue #678).

The approach followed was to use implement the option of abi checking in the build_and_install.py script used by the devel scripts. I've implement an argument in the script named --abi-checker that runs the auto-abi-checker tool using:

  1. The workspace installation space
  2. The debian packages associated to the ROS repository under testing.

Testing:

  • Using the build_and_install.py script directly on my local system the runs of simple packages (rclcpp) was succesful.
  • I'm not sure about the best wait to test this in the ROS2 build farm. Last time I used a testing instance of a buildfarm (I think @nuclearsandwich is generating one these days). If there is a faster approach, please let me know.

Open questions and TODO:

  • Still need to resolve the question in Implementation of abichecker for ROS2 #678 about how are we going to identify which packages needs ABI. The easiest thing would be the abi-checker always. If the package does not provide any .h or .so the checker will probably do nothing and report success.
  • Export results in Jenkins (the osrf buildfarm uses HTML publisher + Console parser to make build unstable.)
  • The local test build fails miserably on packages that has msgs generation support due to the lack of headers related to context and opensplice (not sure if the buildfarm is installing all the support for them). Support to remove the headers of these DDS implementations can be added to the auto-abi-checker.
  • There are problems with packages (i.e: gazebo_ros_pkgs) that introduce boost headers into the mix of headers to check (some expected definitions are missing). I'm still investigating them.

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave it up to @nuclearsandwich to recommend how to test deploy this.

ros_buildfarm/workspace.py Outdated Show resolved Hide resolved
ros_buildfarm/workspace.py Outdated Show resolved Hide resolved
scripts/devel/build_and_install.py Outdated Show resolved Hide resolved
scripts/devel/build_and_install.py Outdated Show resolved Hide resolved
scripts/devel/build_and_install.py Outdated Show resolved Hide resolved
scripts/devel/build_and_install.py Show resolved Hide resolved
scripts/devel/create_devel_task_generator.py Outdated Show resolved Hide resolved
scripts/devel/create_devel_task_generator.py Outdated Show resolved Hide resolved
@dirk-thomas
Copy link
Member

Btw. the current state severely breaks CI which needs to be address before it even makes sense to test deploy this.

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from one comment which can go either way, I think this is ready. Thanks for iterating @j-rivero.

.travis.yml Outdated Show resolved Hide resolved
@j-rivero
Copy link
Contributor Author

Not sure what happened with Travis but seems like it is not running on my last changes. I can promise that they pass :) .

After/before the merge we need to install the Jenkins plugins:

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@j-rivero Please squash merge whenever is ok (not sure of the Jenkins masters need to be updated before, probably not since no repo uses the new flag?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants