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

Merge upstream main #12

Merged
merged 4 commits into from
Nov 20, 2024
Merged

Conversation

mxgrey
Copy link

@mxgrey mxgrey commented Nov 19, 2024

In #11 I had merged the upstream main branch into the PR, but possibly due to a squash commit the git history did not recognize that these commits were accounted for after the squash happened.

This PR doesn't change any lines of code but will allow git to recognize that these commits are accounted for, allowing for a nicer diff.

Please do a regular merge commit (not a squash commit) on this or else it won't have the desired effect.

mxgrey and others added 4 commits October 30, 2024 12:59
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <greyxmike@gmail.com>
* Use latest stable Rust CI

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Fix quotation in doc

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

* Fix serde feature for vendored messages

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

* Fix formatting of lists in docs

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

* Update minimum supported Rust version based on the currently used language features

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

* Conform to clippy style guide

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

* Add rust toolchain as a matrix dimension

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Bump declaration of minimum supported rust version

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Limit the scheduled runs to once a week

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Define separate stable and minimal workflows because matrix does not work with reusable actions

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>

* Reduce minimum version to 1.75 to target Noble

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

---------

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
* Update rclrs vendor_interfaces.py script

This updates the vendor_interfaces.py script to also vendor in the
action_msgs and unique_identifier_msgs packages. The script is modified
to always use a list of package names rather than hard-coding the
package names everywhere.

* Silence certain clippy lints on generated code

In case a user enforces clippy linting on these generated packages,
silence expected warnings. This is already the case in rclrs, but should
be applied directly to the generated packages for the sake of downstream
users.

The `clippy::derive_partial_eq_without_eq` lint was already being
disabled for the packages vendored by rclrs, but is now moved to the
rosidl_generator_rs template instead. This is necessary since we always
derive the PartialEq trait, but can't necessary derive Eq, and so don't.

The `clippy::upper_case_acronyms` is new and was added to account for
message type names being upper-case acrynyms, like
unique_identifier_msgs::msg::UUID.

* Update vendored message packages

This updates the message packages vendored under the rclrs `vendor`
module. The vendor_interfaces.py script was used for reproducibility.

The existing packages – rcl_interfaces, rosgraph_msgs, and
builtin_interfaces – are only modified in that they now use the
std::ffi::c_void naming for c_void instead of the std::os::raw::c_void
alias and disable certain clippy lints.

The action_msgs and unique_identifier_msgs packages are newly added to
enable adding action support to rclrs. action_msgs is needed for
interaction with action goal info and statuses, and has a dependency on
unique_identifier_msgs.
@mxgrey
Copy link
Author

mxgrey commented Nov 20, 2024

@geoff-imdex

After #13 was merged, we only see one commit 622e887 show up in the upstream PR, which suggests that was a squash-merge instead of a merge-commit.

Are you certain that you did a merge-commit of #13 or did you possibly do a squash commit? If that was a squash commit then you'll need to reopen this PR and do a merge-commit of it, otherwise the PR in ros2-rust will be very difficult for them to review since it will contain a lot of extraneous diffs from previous PRs.

@mxgrey
Copy link
Author

mxgrey commented Nov 20, 2024

Just to be clear, this PR has a diff of zero lines, but that's on purpose. The important thing is we need your main branch to recognize the individual commits that already exist on the upstream ros2-rust main branch, otherwise GitHub will not be able to produce an accurate code diff for reviewers.

In order to have the individual commits on your main branch, this PR cannot be merged with the squash option, it needs to be merged as a regular merge-commit.

@mxgrey
Copy link
Author

mxgrey commented Nov 20, 2024

In case it helps to clarify: When you see the option to squash merge the PR, click the arrow button next to it and select Create a merge commit, as highlighted here:

merge-commit

@geoff-imdex
Copy link
Owner

Would it be better if sync'd my master "fork" with the upstream branch - this should allow git to determine that the changes are the same (and hopefully not create a load of conflicts) and reduce the set of changes to the pertinent ones?
If needs be I can do as you suggest and create a merge-commit.
And apologies for the broken process - I've blindly followed a process I've been using for years without really thinking how the upstream commits were being brought in to my PR and the effect that would have.

@mxgrey
Copy link
Author

mxgrey commented Nov 20, 2024

Would it be better if sync'd my master "fork" with the upstream branch

If you mean doing something like $ git merge upstream/main into your fork's main branch, then that will have the exact same effect as doing a merge-commit of this PR, so that would be fine. If you're talking about doing something else, I would advise against it.

this should allow git to determine that the changes are the same (and hopefully not create a load of conflicts) and reduce the set of changes to the pertinent ones?

I don't think there's any way to get around GitHub having the wrong diff without the commits from main being present on your branch. There aren't merge conflicts currently and there still won't be merge conflicts if you just do a commit-merge of this PR.

And apologies for the broken process

Git is very confusing tool and even my own mastery of it is limited, so no worries. I'm just trying to make sure that it's easy as possible for the maintainers to review the PR, and until we get these specific commits merged in properly, the maintainers will have thousands of extraneous lines in the diff which are actually coming from other PRs that were already approved and merged.

@geoff-imdex
Copy link
Owner

I was referring to this GitHub feature
image, which no doubt does the git merge behind the scenes. In retrospect, I should have done this first before applying your commits as this would have tidied things up.
Given where things are at, I'll follow your suggested process - and I've just realised you added a helpful comment at the top of this and PR13 - or at least it would have been helpful if I'd read them (face meet palm :))

@geoff-imdex geoff-imdex reopened this Nov 20, 2024
@geoff-imdex geoff-imdex merged commit 1d661bc into geoff-imdex:main Nov 20, 2024
12 checks passed
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