Regression/Error: "Only one <build_type> element is permitted".

Question

Regression/Error: "Only one <build_type> element is permitted".

facontidavide opened this issue 3 years ago · 11 comments

I think this was introduced in version 0.5.0

This doesn't work anymore:

  <export>
      <build_type condition="$ROS_VERSION == 2">ament_cmake</build_type>
      <build_type condition="$ROS_VERSION == 1">catkin</build_type>
  </export>

If I run echo $ROS_VERSION I can see either 1 or 2

Error message:

Repository type: git
Found packages: behaviortree_cpp_v3
Traceback (most recent call last):
  File "/usr/bin/catkin_prepare_release", line 33, in <module>
    sys.exit(load_entry_point('catkin-pkg==0.5.0', 'console_scripts', 'catkin_prepare_release')())
  File "/usr/lib/python3/dist-packages/catkin_pkg/cli/prepare_release.py", line 217, in main
    _main()
  File "/usr/lib/python3/dist-packages/catkin_pkg/cli/prepare_release.py", line 272, in _main
    build_type = package.get_build_type()
  File "/usr/lib/python3/dist-packages/catkin_pkg/package.py", line 167, in get_build_type
    raise InvalidPackage('Only one <build_type> element is permitted.', self.filename)
catkin_pkg.package.InvalidPackage: Error(s) in package '././package.xml':
Only one <build_type> element is permitted.

Answer 1 · 2022-05-21T19:52:24.000Z

Probably related to #331 (@wjwwood FYI).

I can take a closer look on Monday.

Answer 2 · 2022-05-21T22:02:00.000Z

Yes, I think it was exposed by that pr but all we did was use the existing method for extracting the build type rather than manually extracting it. But we didn't change the method that accesses the build type.

So I guess the official api considers that invalid or maybe it needs to be invoked differently. I'll have to look into more to know what is the right.

At a higher level how should the tool proceed if in one situation it's a support build type but in another it is not supported? Does consider both build type tags? Should the user be required to set the ROS version (in this case) as an ebb car before invoking the tool?

Answer 3 · 2022-05-23T17:00:16.000Z

So I guess the official api considers that invalid or maybe it needs to be invoked differently. I'll have to look into more to know what is the right.

The Package.get_build_type() function implicitly expects that either there is exactly one unconditional build type or that package conditions are evaluated before getting the build type.

The version check scripts don't otherwise necessitate an active ROS context as far as I know.

I can see a few possible ways to resolve this:

Circumvent the accessor function and check all exported build types
Evaluate package conditions using environment context (if there is one) or a fallback context
Catch the error and require the version bump to be run in an environment context.

It occurs to me that we could "just not support" multiple build type packages.
This seems initially attractive if you assume that ROS 1's star is fading but I think that it will be some time before it goes out completely and it implies that we don't expect to deal with any future build system transitions. While none are currently anticipated ROS transitioned from rosbuild to catkin and it makes sense to at least consider the possibility that ROS 2 may require a transition in the future.

At the moment, the only build type dependent behavior is for the ament_python build type. I've never seen a package with the build type

<export>
  <build_type condition="$ROS_VERSION == 2">ament_python</build_type>
  <build_type condition="$ROS_VERSION == 1">catkin</build_type>
</export>

But if there's one out there, it represents a case where the update functions would actually do different things depending on the context.

Answer 4 · 2022-05-23T17:44:05.000Z

At the moment, the only build type dependent behavior is for the ament_python build type. I've never seen a package with the build type
...
But if there's one out there, it represents a case where the update functions would actually do different things depending on the context.

Well, ALL my packages (plotjuggler, behaviortree, etc) build like that and the DO different things. This new regression is blocking be from creating new releases.

It occurs to me that we could "just not support" multiple build type packages.

That will be hugely annoying for me

Answer 5 · 2022-05-23T23:17:29.000Z

That will be hugely annoying for me

The feedback is definitely heard and appreciated. I wanted to make it explicit that I don't consider not supporting this case a viable option which I can't do if I don't mention it at all. My apologies if I gave the impression that it was under consideration.

Well, ALL my packages (plotjuggler, behaviortree, etc) build like that and the DO different things.

I'm confused, aren't all your packages conditionally using catkin or ament_cmake? The case I described above is conditional between catkin and ament_python.

Answer 6 · 2022-05-24T03:25:29.000Z

Circumvent the accessor function and check all exported build types

Spiked this out in #337 just to see it work.

Answer 7 · 2022-05-25T17:05:58.000Z

One Weird Thing:tm: I noticed while poking around for #337 was that the catkin_package_version command worked just fine on a package with conditional build types.

Given the package.xml:

<?xml version="1.0"?>
<?xml-model href="http://download.ros.org/schema/package_format3.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
<package format="3">
  <name>canary_condition</name>
  <version>0.8.0</version>
  <description>Canary package to test conditional dependencies.</description>
  <maintainer email="steven@openrobotics.org">Steven! Ragnarök</maintainer>
  <license>Apache License 2.0</license>


  <depend condition="$ROS_VERSION == 2">ament_cmake</depend>
  <depend condition="$ROS_VERSION == 1">catkin</depend>
  <depend condition="$ROS_PYTHON_VERSION == 3">python3-catkin-pkg</depend>
  <depend condition="$ROS_PYTHON_VERSION == 2">python-catkin-pkg</depend>

  <export>
    <build_type condition="$ROS_VERSION == 2">ament_cmake</build_type>
    <build_type condition="$ROS_VERSION == 1">catkin</build_type>
  </export>
</package>

the command catkin_package_version --bump minor worked just fine:

❯ catkin_package_version --bump minor
0.8.0 -> 0.9.0

However, when I change one of the build types to ament_python, it breaks.
With this package.xml

<?xml version="1.0"?>
<?xml-model href="http://download.ros.org/schema/package_format3.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
<package format="3">
  <name>canary_ament_python</name>
  <version>0.7.1</version>
  <description>Canary package to test conditional dependencies.</description>
  <maintainer email="steven@openrobotics.org">Steven! Ragnarök</maintainer>
  <license>Apache License 2.0</license>


  <depend condition="$ROS_VERSION == 2">ament_cmake</depend>
  <depend condition="$ROS_VERSION == 1">catkin</depend>
  <depend condition="$ROS_PYTHON_VERSION == 3">python3-catkin-pkg</depend>
  <depend condition="$ROS_PYTHON_VERSION == 2">python-catkin-pkg</depend>

  <export>
    <build_type condition="$ROS_VERSION == 2">ament_python</build_type>
    <build_type condition="$ROS_VERSION == 1">catkin</build_type>
  </export>
</package>

❯ catkin_package_version --bump minor
Error(s) in package '././package.xml':
Only one <build_type> element is permitted.

I ended up chucking out the command-wide exception handler so I could see the full stacktrace:

❯ catkin_package_version --bump minor
Traceback (most recent call last):
  File "/home/steven/osrf/catkin_pkg/.direnv/python-3.10.4/bin/catkin_package_version", line 33, in <module>
    sys.exit(load_entry_point('catkin-pkg', 'console_scripts', 'catkin_package_version')())
  File "/home/steven/osrf/catkin_pkg/src/catkin_pkg/cli/package_version.py", line 39, in main
    raise e
  File "/home/steven/osrf/catkin_pkg/src/catkin_pkg/cli/package_version.py", line 36, in main
    update_versions(packages, new_version)
  File "/home/steven/osrf/catkin_pkg/src/catkin_pkg/package_version.py", line 152, in update_versions
    build_type = package_obj.get_build_type()
  File "/home/steven/osrf/catkin_pkg/src/catkin_pkg/package.py", line 167, in get_build_type
    raise InvalidPackage('Only one <build_type> element is permitted.', self.filename)
catkin_pkg.package.InvalidPackage: Error(s) in package '././package.xml':
Only one <build_type> element is permitted.

In both packages, catkin_prepare_release --bump minor --no-push has the same result:

❯ catkin_prepare_release --bump minor --no-push
Prepare the source repository for a release.
Repository type: git
Found packages: canary_condition
Traceback (most recent call last):
  File "/home/steven/osrf/catkin_pkg/.direnv/python-3.10.4/bin/catkin_prepare_release", line 33, in <module>
    sys.exit(load_entry_point('catkin-pkg', 'console_scripts', 'catkin_prepare_release')())
  File "/home/steven/osrf/catkin_pkg/src/catkin_pkg/cli/prepare_release.py", line 217, in main
    _main()
  File "/home/steven/osrf/catkin_pkg/src/catkin_pkg/cli/prepare_release.py", line 272, in _main
    build_type = package.get_build_type()
  File "/home/steven/osrf/catkin_pkg/src/catkin_pkg/package.py", line 167, in get_build_type
    raise InvalidPackage('Only one <build_type> element is permitted.', self.filename)
catkin_pkg.package.InvalidPackage: Error(s) in package '././package.xml':
Only one <build_type> element is permitted.

❯ catkin_prepare_release --bump minor --no-push
Prepare the source repository for a release.
Repository type: git
Found packages: canary_ament_python
Traceback (most recent call last):
  File "/home/steven/osrf/catkin_pkg/.direnv/python-3.10.4/bin/catkin_prepare_release", line 33, in <module>
    sys.exit(load_entry_point('catkin-pkg', 'console_scripts', 'catkin_prepare_release')())
  File "/home/steven/osrf/catkin_pkg/src/catkin_pkg/cli/prepare_release.py", line 217, in main
    _main()
  File "/home/steven/osrf/catkin_pkg/src/catkin_pkg/cli/prepare_release.py", line 272, in _main
    build_type = package.get_build_type()
  File "/home/steven/osrf/catkin_pkg/src/catkin_pkg/package.py", line 167, in get_build_type
    raise InvalidPackage('Only one <build_type> element is permitted.', self.filename)
catkin_pkg.package.InvalidPackage: Error(s) in package '././package.xml':
Only one <build_type> element is permitted.

It looks like there may be two separate code paths for bumping versions and #331 only covered one.

This still doesn't give any insight into how best to resolve the underlying error but I wanted to untangle it so we solve the problem in the right place.

Answer 8 · 2022-05-25T18:18:21.000Z

From our last offline conversation @cottsay's in favor of trying to resolve conditions using the current environment and if that fails, fall back to a blind guess (i.e. pick the first one).

I've pedaled it out some and I think it actually makes more sense to unconditionally check all build types and treat the package as an ament_python package if that build type is present. Here's why:

If we consider, catkin, cmake, and ament_cmake to all share the "default" bump behavior, then we currently only really care if ament_python is one of a package's conditional build types and need to take special action there.

There has been no previous requirement that catkin_prepare_release or catkin_package_version be run within the context of an existing rosdistro and I think the likelihood that we would end up being run in the "wrong" context is higher than the benefit from being run in context. For a single-source package using conditional build types it isn't even clear what the "correct" context is? Maintainers would need to be aware that running in a context that evaluates the build type to ament_python (currently) triggers a superset of the behaviors for other build types. This problem will only get worse as different build types require different, possibly (albeit unlikely) conflicting transformations in order to produce a release / version bump. I think it's easier on the maintainer to do the right thing for all build types rather than picking one and try to do the right thing for it.

We'll likely need #337 or something like it in order to get info about all build types. On top of that I'll propose (and start to implement later today) a refactoring of the current bump logic to:

Figure out why the codepaths between catkin_package_version and catkin_prepare_release differ and unify them if possible.
Update the logic from #331 so that all build types are considered and required actions are performed for each of them.
Do this in such a way that assumes someday we'll add another build type with another metadata file (like a Rust cargo.toml) and that when it is detected in the list its handling can be added to the above.

@cottsay I'm going to proceed with my followup PRs optimistically hoping I've convinced you but I don't yet consider the matter closed.

Answer 9 · 2022-05-25T21:29:14.000Z

It looks like there may be two separate code paths for bumping versions and #331 only covered one.

This isn't correct. catkin_pkg.package_version.update_versions() is the only method for actually changing the version in files on disk but it checks for the presence of a setup.py file before checking the package build type and thus wasn't hitting problems unless the package had a setup.py.

Answer 10 · 2022-05-25T23:52:36.000Z

@facontidavide we've merged a couple of PRs which together address all known regressions when using catkin_prepare_release and catkin_package_version in packages which contain multiple, conditional build types.

I'm planning to release catkin_pkg 0.5.1 tomorrow which includes those fixes. If you have the opportunity and interest to temporarily install catkin_pkg from master and let us know whether all your issues are resolved that would be most welcome. Thanks again for reporting the issue!

Answer 11 · 2022-06-11T23:01:42.000Z

Hi!

I found exactly the same issue than @facontidavide while trying to release a package intended to support ROS1/ROS2 from one single branch, with:

 <export>
    <build_type condition="$ROS_VERSION == 1">catkin</build_type>
    <build_type condition="$ROS_VERSION == 2">ament_cmake</build_type>
 </export>

Running with catkin-pkg=0.5.0 fails:

$ catkin_prepare_release 
Prepare the source repository for a release.
Repository type: git
Found packages: mrpt_msgs
Traceback (most recent call last):
  File "/usr/local/bin/catkin_prepare_release", line 11, in <module>
    load_entry_point('catkin-pkg==0.5.0', 'console_scripts', 'catkin_prepare_release')()
  File "/usr/local/lib/python2.7/dist-packages/catkin_pkg-0.5.0-py2.7.egg/catkin_pkg/cli/prepare_release.py", line 217, in main
    _main()
  File "/usr/local/lib/python2.7/dist-packages/catkin_pkg-0.5.0-py2.7.egg/catkin_pkg/cli/prepare_release.py", line 272, in _main
    build_type = package.get_build_type()
  File "/usr/local/lib/python2.7/dist-packages/catkin_pkg-0.5.0-py2.7.egg/catkin_pkg/package.py", line 167, in get_build_type
    raise InvalidPackage('Only one <build_type> element is permitted.', self.filename)
catkin_pkg.package.InvalidPackage: Error(s) in package '././package.xml':
Only one <build_type> element is permitted.

while running v0.5.2 works, so I can confirm the new release solved this issue, thanks!!