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

Added adi_3dtof_image_stitching package #43003

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

raebelchristo-adi
Copy link
Contributor

Please add the following dependency to the rosdep database.

Package name:

adi_3dtof_image_stitching

Package Upstream Source:

https://github.com/analogdevicesinc/adi_3dtof_image_stitching/tree/v1.1.0

Purpose of using this:

adi_3dtof_image_stitching dependency allows a user to stitch together multiple depth and IR images published by the ADI's ADTF3175D ToF sensor. This has a significance in wide angle applications to increase the field of view.

Checks

  • All packages have a declared license in the package.xml
  • This repository has a LICENSE file
  • This package is expected to build on the submitted rosdistro

@github-actions github-actions bot added the noetic Issue/PR is for the ROS 1 Noetic distribution label Oct 1, 2024
@sloretz
Copy link
Contributor

sloretz commented Oct 1, 2024

Holding for Noetic sync

@sloretz sloretz added the held for sync Issue/PR has been held because the distribution is in a sync hold label Oct 1, 2024
Copy link
Collaborator

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

The license should be one of the open-source ones listed in https://opensource.org/licenses (preferably one of the "Popular licenses").
However, the license file https://github.com/analogdevicesinc/adi_3dtof_image_stitching/blob/v1.1.0/LICENSE
has an Analog Devices license which is not listed in the https://opensource.org/licenses

@raebelchristo-adi
Copy link
Contributor Author

We have used the BSD-3-Clause license which comes under the "Popular/Strong Community" Category as listed in https://opensource.org/licenses. We only replaced the year and copyright holder placeholders with the corresponding year and the name of our organization.

@sloretz sloretz removed the held for sync Issue/PR has been held because the distribution is in a sync hold label Oct 3, 2024
@MichaelOrlov
Copy link
Collaborator

@raebelchristo-adi I don't know how it happened, but https://github.com/analogdevicesinc/adi_3dtof_image_stitching/blob/v1.1.0/LICENSE have ADI license
image
And this is what gitlab officially shows under the what license this project is.

However, at the same time in the https://github.com/analogdevicesinc/adi_3dtof_image_stitching/blob/v1.1.0/package.xml
mentioned BSD license.
image

The licenses must be the same. Please adjust.

As regards your latest commit with changing the source version to the noetic-devel branch instead of the pinned version with a tag. We usually do not allow this "workaround".

@raebelchristo-adi
Copy link
Contributor Author

The licenses must be the same. Please adjust.

Could it be that the extra lines appended at the bottom of the LICENSE file that makes github think it is an ADI license rather than BSD-3? In that case I might need to check with an internal resource regarding how this can be altered.

As regards your latest commit with changing the source version to the noetic-devel branch instead of the pinned version with a tag. We usually do not allow this "workaround".

We had an edit in the package.xml and CMakelists.txt that was merged into the noetic-devel branch. Hence we wanted this to be the main source branch as all maintenance happens here. Do you have any other suggestions if this workaround is not allowed?

@github-actions github-actions bot added the humble Issue/PR is for the ROS 2 Humble distribution label Oct 7, 2024
@raebelchristo-adi
Copy link
Contributor Author

Regarding license changes. we are reviewing the alterations for the LICENSE file internally. I will update about it here.

@MichaelOrlov MichaelOrlov requested a review from cottsay October 7, 2024 17:41
Copy link
Collaborator

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@sloretz @clalancette Please correct me if I am wrong, if I understand correctly we shouldn't allow sources of the packages to track development branches like https://github.com/analogdevicesinc/adi_3dtof_image_stitching/tree/humble-devel unless some extraordinary cases.
The source: version: should refer to the tag in the repo associated with the release for the package.
Otherwise, if source version will be set to track the development branch and, If it will be a regression, it will be no way to quickly revert changes by the ROS Boss when he preparing be-weekly synch.

@sloretz
Copy link
Contributor

sloretz commented Oct 7, 2024

@MichaelOrlov The source: version: should be the development branch from which release are made rather than a tag. The source block is used by tools like rosinstall_generator --upstream-development to get the latest unreleased sources for a package.

Otherwise, if source version will be set to track the development branch and, If it will be a regression, it will be no way to quickly revert changes by the ROS Boss when he preparing be-weekly synch.

Syncs are only affected by content in the release block.

@MichaelOrlov
Copy link
Collaborator

@sloretz Thanks for the clarification.
@raebelchristo-adi As soon as licensing issue will be resolved this PR will be good to go.

@raebelchristo-adi
Copy link
Contributor Author

The license of the repository has been updated. It now shows as BSD-3-Clause.

@clalancette clalancette merged commit a420664 into ros:master Oct 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
humble Issue/PR is for the ROS 2 Humble distribution noetic Issue/PR is for the ROS 1 Noetic distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants