search

tesseract-robotics / tesseract_ros2

22 stars 24 forks source link

Add IFOPT to rosinstall #20

Closed mpowelson closed 4 years ago

mpowelson commented 4 years ago

This is a new TrajOpt dependency

schornakj commented 4 years ago

I think this is fine for now, but it seems like it would be better if Trajopt could handle fetching all its own dependencies. Should ifopt be added to trajopt_ext?

mpowelson commented 4 years ago

We could, but that makes development more difficult. We are doing the same thing in trajopt where we have descartes_light and opw_kinematics because Tesseract requires them.

schornakj commented 4 years ago

My point of comparison would be the methods used in Tesseract to track dependencies like Bullet and FCL. They're brought in using CMake's ExternalProject function, so I don't need to make sure that every project that uses Tesseract also has Bullet, etc. in its rosinstall file. Trajopt does something similar to bring in OSQP.

Part of why I'm thinking about this is that one of my CI processes failed because the PR with the ifopt dependency was merged in at an inopportune time. Partly my fault for having my rosinstall file point to the master branch instead of a specific commit, but it got me wondering about dependency management.

mpowelson commented 4 years ago

We could add them that way as well. I just did not because unlike bullet it is a catkin package. Using cmake external project has it's downsides as well. The source code ends up being in the build directory where it is easily accidentally cleaned for instance.

Regardless, ifopt is released on the build farm. While I have not asked him, I would assume the author with do a release with the features we need in the next few months based on his past behavior.

gavanderhoorn commented 4 years ago

Part of why I'm thinking about this is that one of my CI processes failed because the PR with the ifopt dependency was merged in at an inopportune time. Partly my fault for having my rosinstall file point to the master branch instead of a specific commit, but it got me wondering about dependency management.

Apologies for being the "grumpy granddad", but this is indeed exactly why specifying specific versions of dependencies should be the default, instead of tracking a moving target.

It seems like an anti-pattern to me to work-around this by just copying all dependencies into your sources (which is sort of what you're doing by adding stuff to trajopt_ext).

If you're worried about reproducability: specify specific versions in your .rosinstall file. If you're worried about continuity: fork an upstream repository (but this does make you responsible for tracking upstream as well).

Levi-Armstrong commented 4 years ago

Apologies everyone, this was an error on our side. We had thought everything was running in CI but it turned out that a few packages were not added to the whitelist in trajopt so everything appeared to be fine but it was not.

It seems like an anti-pattern to me to work-around this by just copying all dependencies into your sources (which is sort of what you're doing by adding stuff to trajopt_ext).

The only reason I prefer AddExternalProject over rosinstall is that you can set specific CMake flags and it supports repositories that do not contain a package.xml. I have not looked into this for rosinstall but if this functionality is supported?

schornakj commented 4 years ago

Apologies for being the "grumpy granddad", but this is indeed exactly why specifying specific versions of dependencies should be the default, instead of tracking a moving target.

Lesson definitely learned!

This probably isn't the right place to have a detailed discussion about dependency management for Tesseract and Trajopt, so I'll post a new issue somewhere else and link to it here.

gavanderhoorn commented 4 years ago

The only reason I prefer AddExternalProject over rosinstall is that you can set specific CMake flags and it supports repositories that do not contain a package.xml. I have not looked into this for rosinstall but if this functionality is supported?

So that's the thing: AddExternalProject(..) allows you to mix the responsibilities of setting up a workspace and building it.

Those are two different things, conceptually as well as concretely, but CMake mixes them with that command (I seem to remember issues reported here having to do with Bullet (fi) always getting downloaded and rebuilt: that would seem to be a good example for why you might not want to mix these two responsibilities).

vcstool (or wstool) is purely a development environment setup tool (or workspace seeding tool). It does not build software.

So, no, vcstool nor wstool support setting build flags in the file which is used to populate a workspace with source artefacts.

I assume if I write it like this, it actually makes sense for that to not be supported :)

Colcon actually supports specifying build flags for packages by including a .mixin with the appropriate keys and values. Those can be stored in the repository and then the responsibilities are appropriately separated (vcstool fetches the sources, colcon builds them, flags are set by reading the .mixins).

mpowelson commented 4 years ago

@Levi-Armstrong Finally passing