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

refactor(ndt_omp): prefix with autoware_ #68

Closed
wants to merge 1 commit into from

Conversation

esteve
Copy link

@esteve esteve commented Sep 17, 2024

This PR prefixes ndt_omp with autoware_

See autowarefoundation/autoware#4569

@esteve esteve marked this pull request as ready for review September 17, 2024 14:20
@esteve
Copy link
Author

esteve commented Sep 17, 2024

@SakodaShintaro can you review this PR when you have a moment? Thanks.

@SakodaShintaro
Copy link

Thank you for the pull request.
The corresponding pull requests for autoware.universe and localization_evaluation_tools (a private TIER IV repository) need to be prepared. I will create these, so please wait one or two days.

@SakodaShintaro
Copy link

@esteve
Sorry, but the situation is a bit complicated as to whether or not to add "autoware_" prefix to ndt_omp, so please consult with @mitsudome-r -san

@esteve
Copy link
Author

esteve commented Sep 18, 2024

@SakodaShintaro I spoke about this with @mitsudome-r and @xmfcx yesterday during the ASWG meeting and they were ok with adding the prefix to ndt_omp. Moreover, we agreed a while ago that any public change shouldn't be blocked by the private development of TierIV or any other company.

@mitsudome-r
Copy link
Collaborator

@esteve I had a discussion with TIER IV localization team regarding the naming. First of all, they are okay with adding autoware_ prefix to the package. However, they were a bit worried about having a similar package registered to ROS Buildfarm with just adding a prefix to the package.

Are there any examples where there is a forked package released as a binary with a different name in ROS Buildfarm?
Also, since the package is just a library used for ndt_scan_matcher, they were also wondering if there is a way to just statically link this library within ndt_scan_matcher and not release this package as binary if it isn't a good thing to release pretty much the duplicate of existing package.

@SakodaShintaro
Copy link

I think that one option could be to abandon this repository and move all the necessary code into the ndt_scan_matcher package in autoware.universe. What do you think about this approach?

@esteve
Copy link
Author

esteve commented Sep 18, 2024

AFAIK the only package that depends on ndt_omp is ndt_scan_matcher, so we could just roll all the code here into that package and not worry about existing packages in the ROS index.

@esteve
Copy link
Author

esteve commented Sep 18, 2024

@SakodaShintaro I can move the code from here to ndt_scan_matcher, but I'd prefer to do it when this repository is archived, to prevent anyone from pushing new commits. Thanks.

@SakodaShintaro
Copy link

There are no particular changes we want to make in the near future in this repository, so I think it will be fine to proceed with the archive and move flow. I will discuss this with the team.

Additionally, there is currently a pull request on autoware.universe to add a prefix to ndt_scan_matcher, so I would like to handle that first:
autowarefoundation/autoware.universe#8904
I think I will be able to get it done by tomorrow.

@mitsudome-r
Copy link
Collaborator

@SakodaShintaro Thanks!

@SakodaShintaro
Copy link

I created a pull request in autoware.universe to visualize the necessary changes:
autowarefoundation/autoware.universe#8912
Based on this PR, after confirming that there are no objections, I will archive the ndt_omp repository and finalize the PR in autoware.universe.

@SakodaShintaro
Copy link

autowarefoundation/autoware.universe#8912 has been merged.

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