Un-needed run_depend on fluid
Closed this issue · 47 comments
I don't think stage_ros needs the run-depend on fluid. fluid is a UI design package like qt designer and qt uic, and none of code in stage or stage_ros uses it. It should be safe to remove.
What is the runtime equivalent for libfltk-dev
in that case? Surely it needs to run depend on something.
It should just be libfltk. Fluid is kind of an ancilliary utility, libfltk contains all of actual UI code. Stage should be pulling in fltk in on its own, as Stage needs the fltk libs to launch its own UI.
Stage should be pulling in fltk in on its own, as Stage needs the fltk libs to launch its own UI.
Since runtime dependencies are explicitly listed in ROS, and the CMakeLists.txt
for this package references FLTK, I would say that this package should also run_depend
on libfltk
.
Can you guys have a look at these related issues and make sure we're on the right track:
ros/rosdistro#7377
ros/rosdistro#6988
I think that there was a reason we started to include fluid
as the runtime of fltk
, but maybe we were mistaken, or maybe it only matters on Ubuntu. I'm not sure.
My guess would be that fluid
was added to the fltk
key only because it appeared that fltk
needed it. In light of the fact that stage doesn't use it, and fltk
can be configured to not look for it in CMake, and the fact that the rosdep key was so conveniently split recently, it looks like it can safely be removed from stage
and stage_ros
.
I think we did make one mistake, however. The Ubuntu package fluid
[1] depends on libfltk
, and by removing that dep from stage
, it no longer looks for libfltk
at install time. It would have been correct to replace fluid
with a new rosdep key for libfltk
(runtime, not dev). That same key should be used here.
@cottsay The reason that probably didn't break anything was because stage_ros
explicitly depends on libfltk-dev
so it didn't need the run_depend
on libfltk
from stage
.
What I'm saying is that fluid
actually has a run_depend
on libfltk
, so it got it indirectly via that path as well. So run_depend
on fluid
works, but run_depend
on libfltk
is more precise (and I don't mean the Ubuntu release 😛).
EDIT: sorry @wjwwood, I should have read my own post for better context. Yes, we're on the same page :)
Okay, I have a rosdep
key ready for libfltk
, but I'm no longer sure that it is right for the job. Both stage
and stage_ros
need libfltk-dev
installed in order to compile against them. Because we can't express "downstream_depends" in package.xml
, we have to add libfltk-dev
to run_depends
to ensure a downstream package that build_depends
on stage_ros
has all of the necessary software installed in order to compile against it.
So I'm now thinking that the fluid
dep should be changed to libfltk-dev
.
Thoughts @wjwwood?
Thanks,
--scott
You would only need to define libfltk-dev
as a run depend for stage
, anyone who build depends on stage_ros
will get the build depends of stage_ros
and the recursive build and run depends of those dependencies.
If you use package.xml
format 2 you can use the build_export_depend
to model the transitive dependency.
You would only need to define libfltk-dev as a run depend for stage, anyone who build depends on stage_ros will get the build depends of stage_ros and the recursive build and run depends of those dependencies.
Right. Should we remove the fluid
depend in stage_ros
and add libfltk-dev
to stage
then?
If you use package.xml format 2 you can use the build_export_depend to model the transitive dependency.
Interesting - I'll have to keep that in mind.
I guess what said above doesn't cover the installed state, but if someone depends on stage_ros
and installs it, then stage_ros
's run depends are installed, which will install stage
, who's run depends will also be installed. So there is no requirement for stage_ros
to build/run depend on libfltk-dev
. However, if stage_ros
uses the fltk headers directly it should build and run depend on libfltk-dev
incase stage suddenly stops depending on them for some reason.
Right. Should we remove the fluid depend in stage_ros and add libfltk-dev to stage then?
Sounds right to me.
However, if stage_ros uses the fltk headers directly it should build and run depend on libfltk-dev incase stage suddenly stops depending on them for some reason.
It does: https://github.com/ros-simulation/stage_ros/blob/master/CMakeLists.txt#L8-L20
I'd suppose we add libfltk-dev
to each then.
EDIT: Well, I'd suppose not the headers. Actually, I think you're right that it doesn't need a runtime dependency.
EDIT: Well, I'd suppose not the headers. Actually, I think you're right that it doesn't need a runtime dependency.
This. However, if stage
does not properly export the include directories and dependency on fltk, but uses the fltk headers in it's own headers and then stage_ros
uses the stage
headers, then stage_ros
may need to depend on and find_package fltk to make up for stage
's deficiency. At some point in the past I think we were using stage from an external repository and this was the case. Since building it ourselves we should have been able to resolve that as patches to stage
though.
However, if stage does not properly export the include directories and dependency on fltk, but uses the fltk headers in it's own headers and then stage_ros uses the stage headers, then stage_ros may need to depend on and find_package fltk to make up for stage's deficiency.
Stage is trying to export the fltk include directories and libraries via stage.pc, but the logic is broken. I will try to fix it up and add it to my current pull request against stage. I'll also try to add proper cmake config scripts for stage.
@jpgr87 That sounds like a good corse of action.
I really appreciate both of you guys taking time to look into this issue.
Stage is trying to export the fltk include directories and libraries via stage.pc, but the logic is broken. I will try to fix it up and add it to my current pull request against stage. I'll also try to add proper cmake config scripts for stage.
So if all goes well, we can remove all mention of fltk
from stage_ros
, as adding the include dirs and libs from stage
will suffice. Yes?
That's the goal, yes.
OK I just pushed an update to my stage pull request with the aforementioned changes. Commit is at richmattes/Stage@54555de if you want to try to backport and test it.
I just applied @jpgr87's commit to the stage
package, and created https://github.com/cottsay/stage_ros/commit/31ef94b26ade8ce0c1a5f4eb1791702b76dc3149 to go with it. Builds of both in Jade succeeded. I'll build Indigo next, but since there are no longer any libraries produced by stage_ros
, I don't see why Indigo would do anything different than Jade. I think it is a good move to get an official cmake file for Stage rather than include one via stage_ros
.
@jpgr87 - Have you considered this approach to joining the lists in the cmake for stage? It seems a little bit cleaner and removes duplicate entries: https://gist.github.com/cottsay/a1307d0135dc24dd1d94
On a related note, @wjwwood, does it sound correct that the downstream packages mentioned in this [1] issue do not need those build_depends
? If so, I'll create a PR.
Thanks all,
--scott
[1] ros-planning/navigation_tutorials#4
EDIT: I mentioned that stage
and stage_ros
worked in Jade. By that I mean that both packages built from source, and built in RPMs, and stageros
started correctly and launched a world.
just applied @jpgr87's commit to the stage package, and created cottsay@31ef94b to go with it.
Nice!
Have you considered this approach to joining the lists in the cmake for stage?
In stage-config.cmake? Or in stage.pc.in? stage.pc.in would be easy, I can just add a remove_duplicates call in CMakeLists.txt before i descend into the loop to transform the lists of include directories and libraries into the include and lib flags. stage-config.cmake contains a remove_duplicates call so that its consumers won't see duplicate paths, but it would be a little cleaner to assemble and prune the libs and include dirs in stage's main CMakeLists.txt. I did it the way I did to keep mods to Stage's CMakeLists.txt files to a minimum.
Do you think this sub-thead should be in the Stage pull request?
In stage-config.cmake? Or in stage.pc.in?
I meant in the CMakeLists.txt
for stage itself, when the list of includes and libs is generated. Using the list as a string automatically drops ;
between the entries, and then you can just use a string replace to make it -Ifoo
or whatever. It avoids a loop, and avoids the leading space. Not a big deal.
Do you think this sub-thead should be in the Stage pull request?
Yes. If there is more to discuss on the matter, lets do it there.
Bump, is there anything I need to do here? What's the status of this?
Thanks for pinging this thread @wjwwood - I completely forgot about it. I'm not sure this was completely resolved. I'll do my best to make time tonight to at least get a feel for where we left off...
--scott
OK, lets put this to rest. I just finished my test builds, and I think I have things straight.
Here is the situation:
stage
needslibfltk-dev
to build, but does not needfluid
at allstage_ros
does NOT needlibfltk-dev
directly, butlibfltk-dev
headers are referenced transitively throughstage
Here is my proposed solution:
- Changes to
stage
:stage
should have run and build dependencies onlibfltk-dev
[1]- Upstream Stage patch [2] should be applied so that FLTK headers and libs are included in
stage.pc
. This same patch also provides an officialstage-config.cmake
- Changes to
stage_ros
:- All references to FLTK should be removed from
package.xml
andCMakeLists.txt
[3] - Bundled
FindStage.config
should be removed in lieu of upstreamstage-config.cmake
[4]
- All references to FLTK should be removed from
Thoughts?
--scott
[1] ros-gbp/stage-release#7
[2] rtv/Stage@54555de
[3] https://github.com/cottsay/stage_ros/commit/2efb65e90c6badca335ef20430ab98600e8a0bb5
[4] https://github.com/cottsay/stage_ros/commit/1fda0f48be13b36970ebe7bb5feda64a0f76735a
Looks good to me
Ok, so I'll:
- Merge ros-gbp/stage-release#7
- Import the patch from upstream to our release repository (because there is no new Stage version): rtv/Stage@54555de
- Release
stage
into jade and indigo andhydro (as long as hydro is still running on the farm) - Merge
#22#25 after CI has caught up (either with hydro orwith an update to indigo) - Release
stage_ros
into the affected ros distros
Sounds good?
Yes, I believe that is a conclusive list.
@cottsay I'm going to do this after the next sync so that we can have some soak time.
I've released stage
:
- Indigo: ros/rosdistro#9470
- Jade: ros/rosdistro#9471
Ok, I've released stage_ros
too:
- Indigo: ros/rosdistro#9472
- Jade: ros/rosdistro#9473
Builds on Fedora came out green. Please let me know if any new issues pop up. Thanks for working with us on this, @wjwwood.
Will do, thanks to you guys too.
@cottsay fyi, the upstream patch on Stage which I included, breaks for OS X:
https://github.com/ros-gbp/stage-release/blob/release/jade/stage/stage-config.cmake.in#L11
That line is too naive, as the file is called libstage.dylib
on OS X. I'll try to patch it.
Related: http://answers.ros.org/question/217881/stage_ros-indigo-building-error/
Too bad - this change almost went off without a hitch.
Sounds like a pretty straightforward problem. Sorry it was overlooked...
I'd suppose fixing upstream stage is the right course of action. Then we can pull the same patch into stage-release
.
Yeah, I'm going to start with a patch to the release repository and then port it upstream. I have a fix locally, just testing more and cleaning up.
I'm running a prerelease now to see if this works on Linux, and then I'll do a release and open an upstream pr.
Ok, I've released the fix.
@wjwwood Unfortunately, that broke Fedora again. It looks like stage_ros
tries to link against libstage.so as the path was in the "built" state, not the "installed" state. I can see why this would work fine in a catkin evironment (even an isolated one), but it breaks when the "build" directory is no longer available...
See http://csc.mcs.sdsmt.edu/jenkins/job/ros-jade-stage-ros_binaryrpm_21_i386/24/consoleFull
Hmm, ping-pong. I was working on a solution this morning, but I ran into a few dead ends. I'll post here when I have fix that I think will work, that way you can give it a whirl before I release this time.
@cottsay I pushed this change:
If you have time to try it that'd be great. If not I can do another release and we can just see if that makes the farm happy.
We were so close: http://jenkins.ros.org/job/ros-indigo-stage_binarydeb_saucy_amd64/19/
The version of cmake on Saucy is too old. I'll look for a more portable way to solve the problem tomorrow.
I have released new versions for Jade and Indigo which hopefully will result in all the farms passing. @cottsay can you let me know how it goes on the Fedora side?
Saucy farm seems to have passed, so Linux and OS X are good again. Looks like Fedora is blue too. Let me know if there is still and issue.