Un-needed run_depend on fluid about stage_ros HOT 47 CLOSED

What is the runtime equivalent for libfltk-dev in that case? Surely it needs to run depend on something.

from stage_ros.

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.

from stage_ros.

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.

from stage_ros.

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.

from stage_ros.

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.

[1] http://packages.ubuntu.com/trusty/fluid

from stage_ros.

@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.

from stage_ros.

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 :)

from stage_ros.

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

from stage_ros.

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.

from stage_ros.

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.

from stage_ros.

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.

from stage_ros.

Right. Should we remove the fluid depend in stage_ros and add libfltk-dev to stage then?

Sounds right to me.

from stage_ros.

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.

from stage_ros.

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.

from stage_ros.

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.

from stage_ros.

@jpgr87 That sounds like a good corse of action.

I really appreciate both of you guys taking time to look into this issue.

from stage_ros.

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?

from stage_ros.

That's the goal, yes.

from stage_ros.

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.

from stage_ros.

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.

from stage_ros.

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?

from stage_ros.

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.

from stage_ros.

Bump, is there anything I need to do here? What's the status of this?

from stage_ros.

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

from stage_ros.

OK, lets put this to rest. I just finished my test builds, and I think I have things straight.

Here is the situation:

• stage needs libfltk-dev to build, but does not need fluid at all
• stage_ros does NOT need libfltk-dev directly, but libfltk-dev headers are referenced transitively through stage

Here is my proposed solution:

• Changes to stage:
  • stage should have run and build dependencies on libfltk-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 official stage-config.cmake
• Changes to stage_ros:
  • All references to FLTK should be removed from package.xml and CMakeLists.txt [3]
  • Bundled FindStage.config should be removed in lieu of upstream stage-config.cmake [4]

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

from stage_ros.

Looks good to me

from stage_ros.

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 and hydro (as long as hydro is still running on the farm)
• Merge #22 #25 after CI has caught up (either with hydro or with an update to indigo)
• Release stage_ros into the affected ros distros

Sounds good?

from stage_ros.

Yes, I believe that is a conclusive list.

from stage_ros.

@cottsay I'm going to do this after the next sync so that we can have some soak time.

from stage_ros.

I've released stage:

• Indigo: ros/rosdistro#9470
• Jade: ros/rosdistro#9471

from stage_ros.

I created #25 which is #22 rebased on top of the fixed travis settings which tests against indigo.

from stage_ros.

Ok, I've released stage_ros too:

• Indigo: ros/rosdistro#9472
• Jade: ros/rosdistro#9473

from stage_ros.

Builds on Fedora came out green. Please let me know if any new issues pop up. Thanks for working with us on this, @wjwwood.

from stage_ros.

Will do, thanks to you guys too.

from stage_ros.

@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/

from stage_ros.

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.

from stage_ros.

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.

from stage_ros.

ros-gbp/stage-release#9 and ros-gbp/stage-release#10

from stage_ros.

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.

from stage_ros.

Ok, I've released the fix.

from stage_ros.

@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

from stage_ros.

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.

from stage_ros.

@cottsay I pushed this change:

ros-gbp/stage-release@2952c76

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.

from stage_ros.

We were so close: http://jenkins.ros.org/job/ros-indigo-stage_binarydeb_saucy_amd64/19/

from stage_ros.

The version of cmake on Saucy is too old. I'll look for a more portable way to solve the problem tomorrow.

from stage_ros.

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?

from stage_ros.

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.

from stage_ros.