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

Copied type_description_interfaces structs (rep2011) #732

Merged

Conversation

emersonknapp
Copy link
Collaborator

Part of ros2/ros2#1159

Reviewers please see scripts/copy_type_description_generated_sources.bash which creates these copies - the manual changes in this review are

  • rosidl_runtime_c/CMakeLists.txt
  • rosidl_runtime_c/docs/FEATURES.md
  • rosidl_runtime_cpp/CMakeLists.txt
  • rosidl_runtime_cpp/docs/FEATURES.md
  • scripts/copy_type_description_generated_sources.bash

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Besides the changes inline, I'm wondering if we should add a git hook or github action to check if someone manually changes these files. I'd prefer the git hook (that way it will inform them locally when they are committing), but if we can't make that happen then a github action to check this would be nice.

scripts/copy_type_description_generated_sources.bash Outdated Show resolved Hide resolved
scripts/copy_type_description_generated_sources.bash Outdated Show resolved Hide resolved
scripts/copy_type_description_generated_sources.bash Outdated Show resolved Hide resolved
scripts/copy_type_description_generated_sources.bash Outdated Show resolved Hide resolved
rosidl_runtime_c/docs/FEATURES.md Show resolved Hide resolved
rosidl_runtime_cpp/docs/FEATURES.md Show resolved Hide resolved
…e copy is necessary

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Collaborator Author

@clalancette I'm thinking about how a hook could check on this. At the end of the day we don't know where the built code being copied actually came from so it can't be fully proofed against fools or ne'erdowells.

Maybe some "fingerprint" separate file that's also checked in - and that file can be just a big mean warning and one piece of information like the git commit of type_description_interfaces that the generated sources came from

# DO NOT EDIT MANUALLY - managed by scripts/copy_type_description_generated_sources.bash
# INTENTIONALLY CHANGING THIS FILE OUTSIDE THE SCRIPT 
# WILL RESULT IN UNDEFINED BEHAVIOR FOR ALL OF ROS 2 CORE
TYPE_DESCRIPTION_INTERFACES_GIT_COMMIT=deadbeef

And then the git hook could simply verify that this file is also changed alongside changes to any of the copied sources?

@emersonknapp
Copy link
Collaborator Author

Ah - I forgot. git hooks aren't checked in, you can't enforce their use by end users except by extra setup steps asking them to set up hooks for their local clone. The only way to make this policy enforceable is definitely a github action

@emersonknapp emersonknapp force-pushed the emersonknapp/type-description-runtime branch 10 times, most recently from c024954 to b255a85 Compare March 28, 2023 00:22
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/type-description-runtime branch from b255a85 to 0071de8 Compare March 28, 2023 00:24
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Mar 28, 2023

OK - I've checked in a github action that looks for changed files, and fails immediately if "fingerprint" is changed without sources, or vice versa. If both are changed, then it looks for the latest commit on rcl_interfaces:rolling and checks for equality on the value in the fingerprint.

Cases testing:

@emersonknapp emersonknapp force-pushed the emersonknapp/type-description-runtime branch from 6d7888c to 24cad69 Compare March 28, 2023 04:02
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp requested review from clalancette and removed request for methylDragon and quarkytale March 28, 2023 04:09
@emersonknapp
Copy link
Collaborator Author

OK, second pass. I've changed the fingerprint file to be an output of hashes of the files that got copied. The builtin check here only checks that the fingerprint file changes when the copied sources do, but the big mean warning should be what warns folks off from changing it manually.

Cases testing:

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/type-description-runtime branch from 968eb20 to 6dbfd35 Compare March 28, 2023 20:47
@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Mar 28, 2023

Gist: https://gist.githubusercontent.com/emersonknapp/583f883f05c5405351ac612c1c628d1f/raw/5d9dbe9f73ad23684d544bf0ace2955160cf2340/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11701

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

arm failed in infrastructure and auto-triggered rebuild

  • Linux-aarch64 Build Status

@clalancette clalancette merged commit 8b27b67 into ros2:rolling Mar 29, 2023
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.

2 participants