From 1770456586aef2f51e3f0f67c9ee72d7e543c50e Mon Sep 17 00:00:00 2001 From: Tim Perkins Date: Mon, 20 Jul 2026 22:28:08 -0400 Subject: [PATCH] Make message generation idempotent This is necessary to prevent an endless reconfigure loop when using Ninja. Because generated messages are both build outputs and configure dependencies, generating them would trigger a reconfigure, which would then generate the messages, and so on, ad infinitum. Fixes #21. --- proto2ros/cmake/proto2ros_generate.cmake | 15 ++++++++- proto2ros/proto2ros/cli/generate.py | 32 +++++++++++++++--- proto2ros/proto2ros/cli/test_generate.py | 41 ++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 6 deletions(-) create mode 100644 proto2ros/proto2ros/cli/test_generate.py diff --git a/proto2ros/cmake/proto2ros_generate.cmake b/proto2ros/cmake/proto2ros_generate.cmake index bb956f3..2c450b7 100644 --- a/proto2ros/cmake/proto2ros_generate.cmake +++ b/proto2ros/cmake/proto2ros_generate.cmake @@ -52,7 +52,10 @@ function(proto2ros_generate target) set(BASE_PATH "${CMAKE_CURRENT_BINARY_DIR}/proto2ros_generate") set(OUTPUT_PATH "${BASE_PATH}/${ARG_PACKAGE_NAME}") - file(REMOVE_RECURSE "${OUTPUT_PATH}") + # NOTE Do not wipe OUTPUT_PATH here. Generated files are both build outputs + # and configure dependencies (via rosidl_generate_interfaces), so deleting and + # regenerating them can trigger an endless reconfigure loop. Orphaned files + # are pruned below, once the manifest is known. file(MAKE_DIRECTORY "${OUTPUT_PATH}") foreach(path ${ARG_PROTO_DESCRIPTORS}) @@ -140,6 +143,16 @@ function(proto2ros_generate target) file(STRINGS "${OUTPUT_PATH}/manifest.txt" output_files) file(RENAME "${OUTPUT_PATH}/manifest.txt" "${OUTPUT_PATH}/manifest.orig.txt") + # Prune orphaned files left over from previous generations + set(_keep_files ${output_files} + "${OUTPUT_PATH}/manifest.txt" "${OUTPUT_PATH}/manifest.orig.txt") + file(GLOB_RECURSE _existing_files "${OUTPUT_PATH}/*") + foreach(_existing ${_existing_files}) + if(NOT _existing IN_LIST _keep_files) + file(REMOVE "${_existing}") + endif() + endforeach() + add_custom_command( OUTPUT ${output_files} COMMAND diff --git a/proto2ros/proto2ros/cli/generate.py b/proto2ros/proto2ros/cli/generate.py index c44d7e4..421d409 100644 --- a/proto2ros/proto2ros/cli/generate.py +++ b/proto2ros/proto2ros/cli/generate.py @@ -26,6 +26,24 @@ ) +def write_text_if_changed(path: pathlib.Path, content: str) -> None: + """Write ``content`` to ``path`` only if it has changed. + + Avoids bumping the file's modification time when the generated content is + unchanged. This is important because generated files, e.g. ``.msg``, are + both build outputs and CMake configure-time dependencies; rewriting them + unconditionally can cause an endless reconfigure loop. + + """ + if path.exists(): + try: + if path.read_text() == content: + return + except (OSError, UnicodeDecodeError): + pass + path.write_text(content) + + def do_generate(args: argparse.Namespace) -> int: """Primary function to execute conversion of protobufs to ros msgs.""" # Fetch baseline configuration. @@ -101,7 +119,10 @@ def do_generate(args: argparse.Namespace) -> int: # Write message specifications to .py file. specifications_python_file = args.output_directory / "specifications.py" if not args.dry: - specifications_python_file.write_text(dump_specifications_python_module(message_specifications, config) + "\n") + write_text_if_changed( + specifications_python_file, + dump_specifications_python_module(message_specifications, config) + "\n", + ) files_written.append(specifications_python_file) messages_output_directory = args.output_directory / "msg" @@ -112,13 +133,14 @@ def do_generate(args: argparse.Namespace) -> int: for message_specification in message_specifications: message_output_file = which_message_specification(message_specification, messages_output_directory) if args.force_message_gen or not args.dry: - message_output_file.write_text(dump_message_specification(message_specification) + "\n") + write_text_if_changed(message_output_file, dump_message_specification(message_specification) + "\n") files_written.append(message_output_file) # Write Python conversion APIs .py file. conversions_python_file = args.output_directory / "conversions.py" if not args.dry: - conversions_python_file.write_text( + write_text_if_changed( + conversions_python_file, dump_conversions_python_module(message_specifications, known_message_specifications, config) + "\n", ) files_written.append(conversions_python_file) @@ -133,8 +155,8 @@ def do_generate(args: argparse.Namespace) -> int: known_message_specifications, config, ) - conversions_hpp_file.write_text(hpp_content + "\n") - conversions_cpp_file.write_text(cpp_content + "\n") + write_text_if_changed(conversions_hpp_file, hpp_content + "\n") + write_text_if_changed(conversions_cpp_file, cpp_content + "\n") files_written.append(conversions_hpp_file) files_written.append(conversions_cpp_file) diff --git a/proto2ros/proto2ros/cli/test_generate.py b/proto2ros/proto2ros/cli/test_generate.py new file mode 100644 index 0000000..e75f880 --- /dev/null +++ b/proto2ros/proto2ros/cli/test_generate.py @@ -0,0 +1,41 @@ +# Copyright (c) 2025 Robotics and AI Institute LLC dba RAI Institute. All rights reserved. + +"""Unit tests for proto2ros.cli.generate.""" + +import pathlib + +from proto2ros.cli.generate import write_text_if_changed + + +def test_write_text_if_changed_creates_file(tmp_path: pathlib.Path) -> None: + """A missing file is created with the requested content.""" + target = tmp_path / "out.txt" + write_text_if_changed(target, "hello\n") + assert target.read_text() == "hello\n" + + +def test_write_text_if_changed_is_noop_when_unchanged(tmp_path: pathlib.Path) -> None: + """Rewriting identical content must not touch the file (stable mtime). + + Generated files are both build outputs and CMake configure-time + dependencies; bumping their mtime on every generation causes an endless + CMake/Ninja reconfigure loop. This guards that regression. + """ + target = tmp_path / "out.txt" + target.write_text("same\n") + before_ns = target.stat().st_mtime_ns + + write_text_if_changed(target, "same\n") + + assert target.read_text() == "same\n" + assert target.stat().st_mtime_ns == before_ns, "unchanged content must not rewrite the file" + + +def test_write_text_if_changed_updates_on_difference(tmp_path: pathlib.Path) -> None: + """Differing content is written out.""" + target = tmp_path / "out.txt" + target.write_text("old\n") + + write_text_if_changed(target, "new\n") + + assert target.read_text() == "new\n"