search

ros-infrastructure / catkin_pkg

Standalone Python library for the catkin build system.
https://github.com/ros/catkin
Other
47 stars 91 forks source link

WIP: Add warning when circular dependency found. #208

Closed 130s closed 6 years ago

130s commented 6 years ago

Issue Even when there are pkgs that include circular dependency, topological_order_packages returns a list without specifically trying to raise an attention. Because of that, call at some downstream functions that don't handle circular dependency fail without meaningful info (e.g. catkin/catkin_tools#430).

Approach Print the name of packages that include circular dependency.

Output Sample

$ catkin build pkg_aa
:
Workspace configuration appears valid.
-------------------------------------------------------------------
[build] Found '22' packages in 0.0 seconds.
Traceback (most recent call last):
File "/usr/bin/catkin", line 9, in <module>
load_entry_point('catkin-tools==0.4.4', 'console_scripts', 'catkin')()
  File "/usr/lib/python2.7/dist-packages/catkin_tools/commands/catkin.py", line 267, in main
    catkin_main(sysargs)
  File "/usr/lib/python2.7/dist-packages/catkin_tools/commands/catkin.py", line 262, in catkin_main
    sys.exit(args.main(args) or 0)
  File "/usr/lib/python2.7/dist-packages/catkin_tools/verbs/catkin_build/cli.py", line 420, in main
    summarize_build=opts.summarize  # Can be True, False, or None
  File "/usr/lib/python2.7/dist-packages/catkin_tools/verbs/catkin_build/build.py", line 305, in build_isolated_workspace
    packages, context, workspace_packages)
  File "/usr/lib/python2.7/dist-packages/catkin_tools/verbs/catkin_build/build.py", line 98, in determine_packages_to_be_built
    workspace_package_names = dict([(pkg.name, (path, pkg)) for path, pkg in ordered_packages])
AttributeError: 'str' object has no attribute 'name'

AFTER (notice there's WARNING)

$ catkin build pkg_aa
:
[build] Found '22' packages in 0.0 seconds.
WARNING: cicular dependency found in ['rqt_exposure_utility']
Traceback (most recent call last):
File "/usr/bin/catkin", line 9, in <module>
load_entry_point('catkin-tools==0.4.4', 'console_scripts', 'catkin')()
  File "/usr/lib/python2.7/dist-packages/catkin_tools/commands/catkin.py", line 267, in main
    catkin_main(sysargs)
  File "/usr/lib/python2.7/dist-packages/catkin_tools/commands/catkin.py", line 262, in catkin_main
    sys.exit(args.main(args) or 0)
  File "/usr/lib/python2.7/dist-packages/catkin_tools/verbs/catkin_build/cli.py", line 420, in main
    summarize_build=opts.summarize  # Can be True, False, or None
  File "/usr/lib/python2.7/dist-packages/catkin_tools/verbs/catkin_build/build.py", line 305, in build_isolated_workspace
    packages, context, workspace_packages)
  File "/usr/lib/python2.7/dist-packages/catkin_tools/verbs/catkin_build/build.py", line 98, in determine_packages_to_be_built
    workspace_package_names = dict([(pkg.name, (path, pkg)) for path, pkg in ordered_packages])
AttributeError: 'str' object has no attribute 'name'

TODO

dirk-thomas commented 6 years ago

I don't think it is the responsibility of catkin_pkg to print a warning here. A caller which expects such a cycle wouldn't be able to use the API without resulting in this error output.

The return value if the API is specifically designed to provide exactly the right information to the caller which contains the set of packages which are part of the cycle. So in my opinion this needs to be addressed on the caller side (in this case in catkin_tools).

dirk-thomas commented 6 years ago

Closing in favor of using the existing return values to handle this in the calling location and print a warning there.