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

Enhancement/use hpp for headers based on PR https://github.com/moveit/moveit2/pull/3113 #997

Merged
merged 3 commits into from
Dec 31, 2024

Conversation

CihatAltiparmak
Copy link
Member

@CihatAltiparmak CihatAltiparmak commented Dec 20, 2024

Description

Addresses #994

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers

@mikeferguson
Copy link
Contributor

Looks like CI is failing because @sea-bass maybe did a force push to his branch of ros2_kortex?

@mikeferguson
Copy link
Contributor

Also - we aren't going to want to merge this until we get the headers actually released onto the binaries (humble is out, Jazzy sync looks to be happening next monday? not sure about rolling)

@CihatAltiparmak
Copy link
Member Author

I agree with you because, as i mentioned, i have to fix documentation so that documentation can show the best practices to new users.

@sea-bass
Copy link
Contributor

sea-bass commented Dec 20, 2024

Looks like CI is failing because @sea-bass maybe did a force push to his branch of ros2_kortex?

I definitely did do that. Many times.

What's the best way to resolve this?

@mikeferguson
Copy link
Contributor

What's the best way to resolve this?

I think we need to rebuild the tutorial docker image - they automatically rebuild on Sunday morning I think, I'm not sure how to trigger it manually...

@sea-bass
Copy link
Contributor

sea-bass commented Dec 21, 2024

What's the best way to resolve this?

I think we need to rebuild the tutorial docker image - they automatically rebuild on Sunday morning I think, I'm not sure how to trigger it manually...

Ah yes, this is done through the moveit repo actions. I'm on it.

Edit: kicked them off at https://github.com/moveit/moveit2/actions/workflows/tutorial_docker.yaml

@sea-bass
Copy link
Contributor

It ended up being deleting the caches in this repo...

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Given both Jazzy and Rolling branches of MoveIt 2 have .hpp files, I am OK approving this as is.

@mikeferguson
Copy link
Contributor

Given both Jazzy and Rolling branches of MoveIt 2 have .hpp files, I am OK approving this as is.

Just a note that Jazzy debs aren't out yet with those headers (ETA monday according to ROS discourse)

@sea-bass
Copy link
Contributor

Yeah we can wait.

I didn't release Rolling by the way... so those binaries won't work next sync unless we do.

@mikeferguson
Copy link
Contributor

I didn't release Rolling by the way... so those binaries won't work next sync unless we do.

Rolling has 2.12.0 in testing, but not yet sync'd (we just didn't put 2.12.1 in, which includes the fix for checkstartstate)

@sea-bass
Copy link
Contributor

Right. I should know better. Never mind!

@CihatAltiparmak
Copy link
Member Author

CihatAltiparmak commented Dec 22, 2024

Hi @sea-bass , I have also updated documentation manually as i did in previous commit. I think we should wait until merging corresponding PR in moveit_visual_tools. Thus we can early detect potential issues in this PR by triggering CI. The documentation doesn't consist of only source codes.

@sea-bass
Copy link
Contributor

Sounds good. If you can tag me in the other related PRs when ready for review, I'll take a look.

But for sure let's merge after the Jazzy sync.

Also, for those other repos that may not have distinct branches between humble and later, there are conditional includes we can add -- like was done in moveit2.

@sea-bass
Copy link
Contributor

Jazzy sync is in -- shall we merge this?

@CihatAltiparmak
Copy link
Member Author

I've updated my branch, this will build package without cache. Let's see if there is any warning in CI. And then good to roll ahead.

@sea-bass sea-bass merged commit cdee39b into moveit:main Dec 31, 2024
9 checks passed
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