Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[migration-tools] Always release source version with updated release increment instead of last_version. #32112

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

nuclearsandwich
Copy link
Member

When performing the Rolling platform migration several repositories had bloom releases which were not committed to the distribution file at the time of the source ref.
As a result the last_version entries for those repositories did not match the release version in the source distribution during migration.
Since the migration tool was using the last_release in order to handle any releases that were not hard coded to a specifc version and/or release tag this created a split between what was performed by git-bloom-release during migration and what was configured in the resulting distribution file.

One example is the ros2_control repository which had a 2.2.0-1 release made in ros2-gbp/ros2_control-release@7504ac3 but which never received a rosdistro PR.
Thus the migration tool released 2.2.0-2 based on the last_release information in the tracks file but the source distribution was anticipating 2.1.0-2.

Of the changes in this commit I am most hesitant about the release_increment handling since this script will eventually generate and force push updated release artifacts and if in a similar situation a 2.1.0-2 was released but not published in the source distribution file its artifacts would be clobbered where they overlapped with the migration-tool's own 2.1.0-2.
However I don't have a method of resolving that situation without searching each release repository's artifacts for a full list of release increments associated with a version.

…t_version.

When performing the Rolling platform migration several repositories had bloom releases which were not committed to the distribution file at the time of the source ref.
As a result the `last_version` entries for those repositories did not match the release version in the source distribution during migration.
Since the migration tool was using the last_release in order to handle any releases that were not hard coded to a specific version and/or release tag this created a split between what was performed by git-bloom-release during migration and what was configured in the resulting distribution file.

One example is the ros2_control repository which had a 2.2.0-1 release made in ros2-gbp/ros2_control-release@7504ac3 but which never received a rosdistro PR.
Thus the migration tool released 2.2.0-2 based on the last_release information in the tracks file but the source distribution was anticipating 2.1.0-2.

Of the changes in this commit I am most hesitant about the release_increment handling since this script will eventually generate and force push updated release artifacts and if in a similar situation a 2.1.0-2 was released but not published in the source distribution file its artifacts would be clobbered where they overlapped with the migration-tool's own 2.1.0-2.
However I don't have a method of resolving that situation without searching each release repository's artifacts for a full list of release increments associated with a version.
Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested a possible approach to avoid the force-push, but either way, I think this should work OK.


if dest_track['release_tag'] == ':{ask}' and 'last_release' in dest_track:
# Override the version for this release to guarantee the same version is released.
dest_track['release_tag_saved'] = dest_track['release_tag']
dest_track['release_tag'] = dest_track['last_release']
write_tracks_file(tracks, f'Update {args.dest} track to release exactly last-released tag.')

# Update release increment for the upcoming release.
release_inc = str(int(source_inc) + 1)
Copy link
Member

@cottsay cottsay Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than overwriting the tag, I'd opt for a release inc which is "newer than both". It should have the same effect without any force pushing.

(I haven't tested this code)

Suggested change
release_inc = str(int(source_inc) + 1)
release_inc = str(max(int(source_inc), int(dest_track['release_inc'])) + 1)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 in case there was a missed push somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still cases this solution wouldn't cover with multiple uncommitted releases.

Using ros2_control as an example again. If our source distribution was at version 2.1.0-1 and the release repository contained tagged but unpublished in rosdistro releases 2.1.0-2 and 2.2.0-1, then the migration tool would see the last release 2.2.0 and release increment "1" in the tracks file and create a release 2.1.0-2 from

    release_inc = str(max(int("1"), int("1")) + 1)

which would clobber the upstream 2.1.0-2.

This is what led me to think that the only reliable way to prevent clobbering unreleased tags is to scan for all tags matching the target release version to pick a strictly greater release increment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - good catch. Well it's still an improvement, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a doubt.

The question hidden in my comment was Should we take the improvement and continue the migration or push for a solution that handles the further edge case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that there's any current repositories with that specific scenario so I would be inclined to take your improvement and proceed with the migration. But I'll add a comment documenting the additional edge case and try to revisit it before we pull this out again for Humble.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your analysis - we can't do better without scanning the tags, which is a bit more work and IMO sort-of reaches further into the repositories than I think the migration tooling should.

+1 to move forward.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in d80eb24.

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with same comment as Scott

…urrent release increment.

Reduce the likelihood that the migration will clobber existing release
tags by incrementing the greater between the source distribution
increment and the current release increment in the Bloom track.
There are still situations where a release could be clobbered which has
been documented but left unhandled.

Co-authored-by: Scott K Logan <logans@cottsay.net>
@nuclearsandwich
Copy link
Member Author

Whoops I pinged for additional review requests just before seeing @cottsay's additional 👍. I've incorporated the suggestion in d80eb24 and I am pretty confident this covers all outstanding affected repositories as is.

@nuclearsandwich nuclearsandwich changed the title [migration-tools] Always release source version and source increment + 1 instead of last_version. [migration-tools] Always release source version with updated release increment instead of last_version. Feb 9, 2022
@nuclearsandwich nuclearsandwich merged commit 38741a4 into master Feb 9, 2022
@nuclearsandwich nuclearsandwich deleted the migration-tools/source-distribution-version branch February 9, 2022 20:01
nuclearsandwich added a commit that referenced this pull request Feb 9, 2022
Some repositories in this migration were affected by the issue described in #32112
Now that the release version handling has been improved we can retry
migrating those repositories by removing the release version as the
script would for a failed bloom release.
nuclearsandwich added a commit that referenced this pull request Feb 9, 2022
Some repositories in this migration were affected by the issue described in #32112
Now that the release version handling has been improved we can retry
migrating those repositories by removing the release version as the
script would for a failed bloom release.
nuclearsandwich added a commit that referenced this pull request Feb 9, 2022
* Remove repositories in preparation for migration.

* Update target platforms for Rolling.

* Update releases after platform migration.

Migration command:

```
python3 migration-tools/migrate-rosdistro.py --source rolling --dest rolling \
  --source-ref 7eafe04 \
  --release-org ros2-gbp \
```

Of 299 repositories, 22 failed to bloom successfully.
This is most frequently caused by missing rosdep dependencies.

The following packages failed to bloom:

* aws-robomaker-small-warehouse-world
* control_box_rst
* fmi_adapter
* fmilibrary_vendor
* gazebo_ros2_control
* gazebo_ros_pkgs
* ign_rviz
* ignition_cmake2_vendor
* ignition_math6_vendor
* locator_ros_bridge
* moveit
* ntpd_driver
* ompl
* rmw_connextdds
* rmw_gurumdds
* ros_ign
* rosidl_typesupport_gurumdds
* rtabmap
* sdformat_urdf
* usb_cam
* warehouse_ros_mongo
* webots_ros2

* Pull releases for repositories with mismatched versions.

Some repositories in this migration were affected by the issue described in #32112
Now that the release version handling has been improved we can retry
migrating those repositories by removing the release version as the
script would for a failed bloom release.

* Update releases from additional packages after migration tools fix.

Using the migration-tool version from 38741a4

Migration command:

```
python3 migration-tools/migrate-rosdistro.py --source rolling --dest rolling \
  --source-ref 7eafe04 \
  --release-org ros2-gbp \
```

These packages did not build (as expected from previous runs)

* aws-robomaker-small-warehouse-world
* control_box_rst
* fmi_adapter
* fmilibrary_vendor
* gazebo_ros2_control
* gazebo_ros_pkgs
* ign_rviz
* ignition_cmake2_vendor
* ignition_math6_vendor
* locator_ros_bridge
* moveit
* ntpd_driver
* ompl
* rmw_connextdds
* rmw_gurumdds
* ros_ign
* rosidl_typesupport_gurumdds
* rtabmap
* sdformat_urdf
* usb_cam
* warehouse_ros_mongo
* webots_ros2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants