python_setup.py: fix build with setuptools v59.0.0 and newer

[shr-project]

• Fail on a multiline distribution package summary pypa/setuptools#2870 doesn't allow newlines
  in description anymore

• e.g. rosmake currently in Noetic has:
  rosmake is a ros dependency aware build tool which can be used to
  build all dependencies in the correct order.

• causing build to fail with:

• cd /jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/git

• mkdir -p /jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/image/usr/opt/ros/noetic/lib/python3.10/site-packages

• /usr/bin/env PYTHONPATH=/usr/opt/ros/noetic/lib/python3.10/site-packages:/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/build/lib/python3.10/site-packages:/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/opt/ros/noetic/lib/python3.10/site-packages CATKIN_BINARY_DIR=/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/build /jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/bin/python3-native/python3 /jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/git/setup.py egg_info --egg-base /jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/build build --build-base /jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/build install --root=/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/image --prefix=/usr/opt/ros/noetic --install-scripts=/usr/opt/ros/noetic/bin
  running egg_info
  writing /jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/build/rosmake.egg-info/PKG-INFO
  Traceback (most recent call last):
  File "/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/git/setup.py", line 11, in
  setup(**d)
  File "/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/lib/python3.10/site-packages/setuptools/init.py", line 153, in setup
  return distutils.core.setup(**attrs)
  File "/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/lib/python3.10/distutils/core.py", line 148, in setup
  dist.run_commands()
  File "/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/lib/python3.10/distutils/dist.py", line 966, in run_commands
  self.run_command(cmd)
  File "/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/lib/python3.10/distutils/dist.py", line 985, in run_command
  cmd_obj.run()
  File "/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/lib/python3.10/site-packages/setuptools/command/egg_info.py", line 292, in run
  writer(self, ep.name, os.path.join(self.egg_info, ep.name))
  File "/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/lib/python3.10/site-packages/setuptools/command/egg_info.py", line 656, in write_pkg_info
  metadata.write_pkg_info(cmd.egg_info)
  File "/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/lib/python3.10/distutils/dist.py", line 1117, in write_pkg_info
  self.write_pkg_file(pkg_info)
  File "/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/lib/python3.10/site-packages/setuptools/dist.py", line 167, in write_pkg_file
  write_field('Summary', single_line(self.get_description()))
  File "/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/recipe-sysroot-native/usr/lib/python3.10/site-packages/setuptools/dist.py", line 151, in single_line
  raise ValueError('Newlines are not allowed')
  ValueError: Newlines are not allowed
  CMake Error at catkin_generated/safe_execute_install.cmake:4 (message):

  execute_process(/jenkins/mjansa/build/ros/webos-noetic-kirkstone/tmp-glibc/work/qemux86_64-webos-linux/rosmake/1.15.8-1-r0/build/catkin_generated/python_distutils_install.sh)
  returned error code
  Call Stack (most recent call first):
  cmake_install.cmake:61 (include)

• there are many ROS packages with newlines, automatically remove newlines here
  and issue a warning for package maintainers to adjust description and
  long_description accordingly

• few examples from current Noetic:
  Summary: 14 tasks failed:
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/ros-comm/rosgraph_1.15.13-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/ros-comm/roslaunch_1.15.13-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/ros/roslib_1.15.8-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/common-msgs/sensor-msgs_1.13.1-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/bond-core/smclib_1.8.6-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/ros-comm/roslz4_1.15.13-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/ros-comm/rosmsg_1.15.13-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/ros/rosmake_1.15.8-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/ros/roscreate_1.15.8-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/ros-comm/rosparam_1.15.13-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/executive-smach/smach_2.5.0-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/marti-common/swri-rospy_2.14.2-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/kdl-parser/kdl-parser-py_1.14.1-1.bb:do_install
  /jenkins/mjansa/build/ros/webos-noetic-kirkstone/meta-ros/meta-ros1-noetic/generated-recipes/cob-command-tools/scenario-test-tools_0.6.21-1.bb:do_install

  but there are probably many more which aren't shown, because the do_install task wasn't executed due to some dependency already failing

Signed-off-by: Martin Jansa martin.jansa@lge.com

[jspricke]

    description = package.description.replace('\n', ' ')
    if len(description) <= 200:
        data['description'] = description
    else:
        data['description'] = description[:197] + '...'

[shr-project]

that's not the same if you want to set long_description only when it's different than description, but you're right that adding

if data['description'] != package.description
    data['long_description'] = package.description

after this section is probably more readable.

Originally I also had the warning which is now in validate() in this section.

[jspricke]

maybe even do:

if len..
else..
data['description'] = data['description'].replace('\n', ' ')

I don't think we should set long_description if it only differs by newlines.

[schlagenhauf]

Newlines in the package description currently break the ros-noetic-desktop installation on Arch Linux, where python-setuptools 59.1.0 is in the system packages. Merging this PR would be greatly appreciated.

[cottsay]

I'm not sure we want to do this. Just because setuptools doesn't support newlines in descriptions doesn't mean we should discourage package manifests from containing them. A quick survey shows that this warning will affect ~15% of packages in ROS 2.

I think it would be better to silently convert the newlines to spaces.

[cottsay]

Additionally, the package name is missing from this format string.

[nuclearsandwich]

+1 on silent conversion. I would even be tempted to version-scope that so it only happens on setuptools >= 59.1 so that packages on current platforms remain unaffected.

[shr-project]

I no longer have test environment for this, won't be able to update the PR properly now.

[nuclearsandwich]

As noted in #326 I think the approach taken there is preferable. All the same, thanks @shr-project and the other contributors for both raising the issue and proposing patches!

[nuclearsandwich]

@cottsay noted that there are packages with different linebreak strategies which would be adversely affected by #326, leading to both of us swinging back to prefer an implementation more like this patch. However this has kicked up a bit of muck and raised deficiencies with even the current handling of package descriptions before either patch.

One example he raised was the description

<description>
  This is a package which
  does stuff.

  It does it very efficiently.
  You should use it.
</description>

which 326 would truncate to

This is a package which

whereas this patch would transform it into

This is a package which   does stuff.    It does it very efficiently.   You should use it.

The reason for the extra spaces is that no work is done to handle the interior indentation. The current implementation strips all exterior whitespace from the description via _get_node_value but does not touch interior whitespace.

catkin_pkg/src/catkin_pkg/package.py

Line 799 in 55db7cd

value = (''.join([n.toxml() for n in node.childNodes])).strip(' \n\r\t')

When figuring out what we should do we turned to both the XML specification which states:

2.10 White Space Handling

In editing XML documents, it is often convenient to use "white space" (spaces, tabs, and blank lines) to set apart the markup for greater readability. Such white space is typically not intended for inclusion in the delivered version of the document. On the other hand, "significant" white space that should be preserved in the delivered version is common, for example in poetry and source code.
An XML processor MUST always pass all characters in a document that are not markup through to the application. A validating XML processor MUST also inform the application which of these characters constitute white space appearing in element content.

...

2.11 End-of-Line Handling
XML parsed entities are often stored in computer files which, for editing convenience, are organized into lines. These lines are typically separated by some combination of the characters CARRIAGE RETURN (#xD) and LINE FEED (#xA).
To simplify the tasks of applications, the XML processor MUST behave as if it normalized all line breaks in external parsed entities (including the document entity) on input, before parsing, by translating both the two-character sequence #xD #xA and any #xD that is not followed by #xA to a single #xA character.

and REP-149

The description of the package. It can consist of multiple lines and may contain XHTML. But depending on where the description is used XML tags and multiple whitespaces might be stripped.

With both of these in mind, I see this as meaning that once we receive the description contents from the XML parser catkin_pkg as the "application" decides how to handle whitespace within the desciption and that according to REP-149 we should be treating it as a block of XHTML, which, like HTML should not treat any whitespace as significant outside of pre-formatted text blocks.

On top of that, we do know that https://index.ros.org is dumping the contents of <description/> directly into an html <div> tag so there is already at least one place where HTML formatting rules are being applied to the description.

To that end, I've opened #329 which starts adding test cases for xhtml-formatted descriptions. From that PR I expect we'll arrive at a standardized way to get a "plaintext" description which we can then use in setup.py files and other places where HTML-formatted descriptions are not expected.

[nuclearsandwich]

#329 has been merged which adds a separate plaintext_description attribute for packages that includes a very minimal plaintext rendering of the xhtml description which is much better suited to being used in setuptools.

[nuclearsandwich]

#326 has been updated to use the plaintext_description field as the base and will take the first line of the plaintext description. I think once that PR is merged, we can close this one as having been superseded.

Since most package.xml descriptions do not include <br> tags it's quite likely that the combined usage of the plaintext-rendered description and keeping only up to the first rendered newline will mean that most packages won't see a difference between #316 and #326 as the newlines in the raw description will not be seen by #326 at all.

I know this one took some time and had a lot of back and forth. My thanks to everyone for their contributions here as I think we not only arrived at a very good solution in #326 but also found an axis for improvement in our description handling everywhere.

[cottsay]

Closed via #326