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

Update repo name and location for acoustic messages #269

Merged

Conversation

rolker
Copy link
Contributor

@rolker rolker commented May 4, 2023

This addresses #268.

@lauralindzey
Copy link
Contributor

In order to future-proof this, you may want to use a specific tag or commit for the marine_msgs repo, rather than having it use main.

@mabelzhang mabelzhang mentioned this pull request Jun 26, 2023
@mabelzhang
Copy link
Contributor

mabelzhang commented Jun 26, 2023

Might fix errors like this one on acoustic_msgs #272
But the package names in hydrographic_msgs have also been changed, right? So acoustic_msgs are no longer that, it has to be changed to marine_acoustic_msgs now?

😅 I got confused in that issue, because now there's another package that's been released and called acoustic_msgs 😅 and that one had a genuine error that we ran into a few weeks back. It could get very confusing for somebody.

Is marine_msgs still for Noetic? I ask because that's what DAVE targets. If you will have different branches, it would also help if we can use the Noetic one here.

I don't have a setup to test it anymore. I don't know if anybody has at this point. So I don't know if we'll be merging PRs. (I could try to make time, but no guarantees.)

@lauralindzey
Copy link
Contributor

@mabelzhang -- just a note that I'm at sea, and will be getting home late August. I tried (and failed) to get marine_acoustic_msgs properly released before I left, so it's close to the top of my stack for when I get home.

But the package names in hydrographic_msgs have also been changed, right? So acoustic_msgs are no longer that, it has to be changed to marine_acoustic_msgs now?

Correct.

@mabelzhang
Copy link
Contributor

mabelzhang commented Jun 26, 2023

Thanks for the quick reply!

I wonder if you could create stub packages with the old name, and have package.xml that install the new packages. That way, it is backward compatible and won't break people who have been using the APL repo. For example, upstream, we had to change the names for a lot of packages during the Ignition->Gazebo rename, and that could have broken a lot of people. But we did it in a way that didn't.

(Not that I'm making a selfish suggestion because we aren't actively maintaining this repo 😅 )

@mabelzhang mabelzhang added bug Something isn't working to_be_discussed To be discussed at next group meeting. labels Oct 31, 2023
Copy link
Collaborator

@woensug-choi woensug-choi left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!
Could you change the version:main to version: tags/v2.0.0? This way we can fix it to the v2.0.0 tag for future-proof
While it gies Head Detached warning, it works.

@mabelzhang
Copy link
Contributor

mabelzhang commented Dec 2, 2023

Ooh that's a good idea.
I think maintainers (you) can probably push to the branch? If you want to just push that and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working to_be_discussed To be discussed at next group meeting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants