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

Depend on ros-noetic-fcl (0.6) in Noetic #2359

Merged
merged 2 commits into from
Oct 12, 2020
Merged

Conversation

rhaschke
Copy link
Contributor

After having fcl 0.6 released into the ROS repos (additionally to fcl 0.5 in the standard Ubuntu/Debian repos) we can switch to this newer version of fcl now.
Probably, Travis will need to be switched to ros-testing (because it's not synced yet).

@rhaschke rhaschke requested a review from v4hn as a code owner October 10, 2020 08:41
@rhaschke rhaschke mentioned this pull request Oct 10, 2020
13 tasks
@v4hn
Copy link
Contributor

v4hn commented Oct 10, 2020 via email

@tylerjw
Copy link
Member

tylerjw commented Oct 11, 2020

@v4hn I think this is needed for releasing on Noetic. I'm going to try to test this today.

@rhaschke
Copy link
Contributor Author

Yes, @tylerjw is right: This is needed to resolve link conflicts on Buster at least.
Regarding build instructions: We could just mention that 0.6.1 is part of the binary distro and thus automatically installed with MoveIt. If one needs even newer fcl (master), one just needs to clone the repo into the MoveIt workspace as well. On Noetic, with this PR here, dependencies should be resolved automatically, as fcl 0.6 provides a correct package.xml already.

@v4hn
Copy link
Contributor

v4hn commented Oct 12, 2020

Ok, as the MoveIt release is apparently still broken in noetic until this is patched this should be merged before the sync.

I'm going to try to test this today.

Thanks @tylerjw! I will leave the review & merge to you then.

Regarding build instructions: We could just mention that 0.6.1 is part of the binary distro and thus automatically installed with MoveIt. If one needs even newer fcl (master), one just needs to clone the repo into the MoveIt workspace as well. On Noetic, with this PR here, dependencies should be resolved automatically, as fcl 0.6 provides a correct package.xml already.

For noetic that's right, but people will still want to build master on melodic as well.

@tylerjw
Copy link
Member

tylerjw commented Oct 12, 2020

I just tested this in 20.04 and found a problem. People who build moveit from source are going to have a problem when they do this:

$ rosdep install --from-paths . --ignore-src -y --rosdistro=noetic
ERROR: the following packages/stacks could not have their rosdep keys resolved
to system dependencies:
moveit_core: Cannot locate rosdep definition for [fcl]

What should we do to get around this? I realize that is because it is in ros-testing, but I'm having trouble figuring out how to get the rosdep tool to install it. I changed my sources file to point to ros-testing but when I try to use rosdep install it still can't find it (It finds that there are no more dependencies to install after I manually install ros-noetic-fcl). For people using moveit from source in noetic should we provide instructions on how to switch to ros-testing? Am I missing something in my setup?

@tylerjw
Copy link
Member

tylerjw commented Oct 12, 2020

After changing to the testing repo and manually installing ros-noetic-fcl it apears to work on 20.04. I'm going to test doing the same on buster but I'm assuming it'll just work.

The largest issue I see is we should provide instructions for anyone building master on noetic for how to get the ros-noetic-fcl package until it is in the main repos.

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

I added the dependency to the source instructions in this PR. I'm not sure how to make travis use ros-testing. @rhaschke could you make that change as part of this PR?

I also tested this in Buster and it works the same as in 20.04.

@rhaschke
Copy link
Contributor Author

The issue of rosdep not knowing about the fcl key will automatically resolve with the next sync.
Hence (if at all) a comment in the build instructions to install ros-noetic-fcl manually from ros-testing is required.

@tylerjw
Copy link
Member

tylerjw commented Oct 12, 2020

Hence (if at all) a comment in the build instructions to install ros-noetic-fcl manually from ros-testing is required.

Isn't that exactly what my PR to the website is?

- ROS_REPO=ros
- ROS_REPO=ros-testing
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't realize that was what this variable was for. That is nice, thank you.

@rhaschke
Copy link
Contributor Author

Isn't that exactly what my PR to the website is?

Yes, sorry. I only coarse skimmed over the past conversation and missed the PR link.

I'm not sure how to make travis use ros-testing. @rhaschke could you make that change as part of this PR?

Done.

@tylerjw
Copy link
Member

tylerjw commented Oct 12, 2020

I think in the conversation before there was talk of adding it to the dependencies build instructions. I looked over those and thought it made more sense in the source build section to mention it because that's where someone might encounter an issue.

@codecov

This comment has been minimized.

@tylerjw tylerjw merged commit e236475 into moveit:master Oct 12, 2020
@rhaschke rhaschke deleted the fix-fcl branch October 13, 2020 16:24
@tylerjw tylerjw mentioned this pull request Oct 23, 2020
57 tasks
sjahr added a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
Bumps [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) from 2 to 3.
- [Release notes](https://github.com/docker/setup-buildx-action/releases)
- [Commits](docker/setup-buildx-action@v2...v3)

---
updated-dependencies:
- dependency-name: docker/setup-buildx-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai>
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.

None yet

3 participants