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

Patch dependencies and rebuild inside the Minimal CI #451

Closed
wants to merge 8 commits into from

Conversation

mxgrey
Copy link
Collaborator

@mxgrey mxgrey commented Dec 19, 2024

This is an approach for solving #449 .

There are several thorny problems at play that rule out many ideal/preferable solutions. I don't love the approach of this PR, but I think it has the best balance of being simple and not having negative side-effects for users.

The thorny problems:

  1. Semantic versioning in Rust does not account for compatibility with certain compiler versions, so cargo can do a rug pull on our Minimal version CI when a dependency releases a new API-compatible update that depends on more recent language features.
  2. Freezing overly specific versions of dependencies in our Cargo.toml could create conflicts for downstream users who want to benefit from newer versions of libraries and newer language features.
  3. action-ros-ci runs through a very rigid sequence of cloning a fresh copy of our repo and trying to build it without giving us an opportunity to patch the repo beforehand.

This PR tackles all of this in a relatively simple workflow which is only applied to the Minimal version CI:

  1. Run action-ros-ci as normal, except use continue-on-error: true so that the workflow can keep running even after it encounters the incompatible dependency error that we know it will.
  2. Add a step to go into rclrs and explicitly $ cargo add home@=0.5.9 to lock in a specific version of the otherwise incompatible dependency
  3. Rebuild the colcon workspace with the dependency now set to a compatible version
  4. Continue with the rest of the CI as normal

Advantages of this approach:

  • Minimal CI is fixed
  • If any more incompatible dependencies pop up in the future, we can just set specific versions for them in the existing $ cargo add command
  • This has absolutely no effect on downstream users of rclrs

Disadvantages of this approach:

  • Using continue-on-error: true for action-ros-ci feels a little icky to me. We could end up with some confusing errors if that step ever encounters an error besides that one that we're expecting. But we'll never get a false positive for the overall CI since we will still be running tests on all the ros2_rust packages.
  • We should periodically audit whether these dependencies that we're patching are still being used by rclrs or transitively by an rclrs dependency. Otherwise we may start to accumulate superfluous dependencies in the CI.

@mxgrey
Copy link
Collaborator Author

mxgrey commented Dec 23, 2024

Closing this in favor of #452 since #452 is a simpler fix.

We can keep this approach in mind if any other dependency starts using newer language features.

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.

1 participant