Un-needed run_depend on fluid

Question

Un-needed run_depend on fluid

Closed this issue 9 years ago · 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.

Answer 1 · 2015-03-25T02:12:28.000Z

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

Answer 2 · 2015-03-25T02:23:08.000Z

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.

Answer 3 · 2015-03-25T03:35:23.000Z

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.

Answer 4 · 2015-03-25T03:47:14.000Z

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.

Answer 5 · 2015-03-25T04:00:07.000Z

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

Answer 6 · 2015-03-25T04:03:22.000Z

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

Answer 7 · 2015-03-25T04:07:27.000Z

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

Answer 8 · 2015-03-28T18:17:39.000Z

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

Answer 9 · 2015-03-28T20:41:06.000Z

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.

Answer 10 · 2015-03-28T20:43:39.000Z

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.

Answer 11 · 2015-03-28T20:44:19.000Z

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.

Answer 12 · 2015-03-28T20:44:50.000Z

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

Sounds right to me.

Answer 13 · 2015-03-28T20:46:01.000Z

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.

Answer 14 · 2015-03-28T22:13:18.000Z

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.

Answer 15 · 2015-03-28T22:42:11.000Z

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.

Answer 16 · 2015-03-28T22:44:49.000Z

@jpgr87 That sounds like a good corse of action.

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

Answer 17 · 2015-03-28T22:48:03.000Z

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?

Answer 18 · 2015-03-28T22:50:38.000Z

That's the goal, yes.

Answer 19 · 2015-03-29T00:35:55.000Z

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.

Answer 20 · 2015-03-29T23:06:44.000Z

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.

Answer 21 · 2015-03-29T23:24:07.000Z

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?

Answer 22 · 2015-03-29T23:28:48.000Z

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.

Answer 23 · 2015-06-01T19:29:28.000Z

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

Answer 24 · 2015-06-01T19:35:22.000Z

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

Answer 25 · 2015-09-06T04:28:00.000Z

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

Answer 26 · 2015-09-06T16:47:21.000Z

Looks good to me

Answer 27 · 2015-09-10T18:03:18.000Z

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?

Answer 28 · 2015-09-10T19:15:42.000Z

Yes, I believe that is a conclusive list.

Answer 29 · 2015-09-16T00:40:51.000Z

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

Answer 30 · 2015-09-16T21:32:51.000Z

I've released stage:

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

Answer 31 · 2015-09-16T22:54:24.000Z

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

Answer 32 · 2015-09-17T02:02:23.000Z

Ok, I've released stage_ros too:

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

Answer 33 · 2015-09-17T04:04:13.000Z

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

Answer 34 · 2015-09-17T04:32:56.000Z

Will do, thanks to you guys too.

Answer 35 · 2015-09-21T19:39:11.000Z

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

Answer 36 · 2015-09-21T20:15:25.000Z

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.

Answer 37 · 2015-09-21T20:16:12.000Z

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.

Answer 38 · 2015-09-21T20:19:41.000Z

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

Answer 39 · 2015-09-21T20:23:44.000Z

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.

Answer 40 · 2015-09-21T23:24:56.000Z

Ok, I've released the fix.

Answer 41 · 2015-09-22T00:52:28.000Z

@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

Answer 42 · 2015-09-22T19:55:43.000Z

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.

Answer 43 · 2015-09-22T20:17:54.000Z

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

Answer 44 · 2015-09-23T07:15:51.000Z

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

Answer 45 · 2015-09-23T07:19:07.000Z

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

Answer 46 · 2015-09-24T02:02:50.000Z

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?

Answer 47 · 2015-09-24T07:16:32.000Z

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.