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

Generate messages in downstream builds #338

Closed
wants to merge 9 commits into from

Conversation

mjcarroll
Copy link
Contributor

This is a rework of how message generation works. Instead of generating source/headers from proto files as part of building the gz-msgs package, it will instead install scripts and cmake helper functions to allow downstream packages to generate messages as part of their builds.

An example of a downstream package utilizing this: https://github.com/mjcarroll/mjcarroll-custom-msgs

This is a placeholder PR (keeping in draft) so that I can clean my changes up and get feedback.

* Split into MessageFactory, which is a non-static variant, should help
  with any potential ODR issues down the road
* Refactor Factory to just be wrapper around a message factory singleton
* Remove filesystem.hh
* General cleanups

Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label May 11, 2023
Signed-off-by: Michael Carroll <[email protected]>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I know is still on draft but I made some generic comments

# OUTPUT_CPP_HH_VAR - A CMake variable name containing a list that the C++ header path should be appended to
# OUTPUT_CPP_CC_VAR - A Cmake variable name containing a list that the C++ source path should be appended to
# Multi value arguments
# INPUT_PROTOS - Passed to protoc --proto_path
Copy link
Contributor

Choose a reason for hiding this comment

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

indent comment

Comment on lines +54 to +57
endforeach()


set(GENERATE_ARGS
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
endforeach()
set(GENERATE_ARGS
endforeach()
set(GENERATE_ARGS

Comment on lines +68 to +69
DEPENDS
${depends_index}
Copy link
Contributor

Choose a reason for hiding this comment

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

style

Suggested change
DEPENDS
${depends_index}
DEPENDS ${depends_index}

# PROTOC_EXEC - Path to protoc
# INPUT_PROTO - Path to the input .proto file
# OUTPUT_CPP_DIR - Path where C++ files are saved
# OUTPUT_INCLUDES - A CMake variable name containing a list that the C++ header path should be appended to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# OUTPUT_INCLUDES - A CMake variable name containing a list that the C++ header path should be appended to
# OUTPUT_INCLUDES - A CMake variable name containing a list that the C++ include path should be appended to

# PROTOC_EXEC - Path to protoc
# INPUT_PROTO - Path to the input .proto file
# OUTPUT_CPP_DIR - Path where C++ files are saved
# OUTPUT_INCLUDES - A CMake variable name containing a list that the C++ header path should be appended to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# OUTPUT_INCLUDES - A CMake variable name containing a list that the C++ header path should be appended to
# OUTPUT_INCLUDES - A CMake variable name containing a list that the C++ include path should be appended to

@@ -0,0 +1,87 @@
/*
* Copyright (C) 2016 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (C) 2016 Open Source Robotics Foundation
* Copyright (C) 2023 Open Source Robotics Foundation

@@ -0,0 +1,61 @@
/*
* Copyright (C) 2016 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (C) 2016 Open Source Robotics Foundation
* Copyright (C) 2023 Open Source Robotics Foundation

@@ -0,0 +1,107 @@
/*
* Copyright (C) 2016 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Copyright (C) 2016 Open Source Robotics Foundation
* Copyright (C) 2023 Open Source Robotics Foundation

add_subdirectory(integration)
add_subdirectory(performance)
add_subdirectory(regression)
#add_subdirectory(gtest_vendor)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

@@ -0,0 +1,165 @@
#!/usr/bin/env python3
#
# Copyright (C) 2022 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright (C) 2022 Open Source Robotics Foundation
# Copyright (C) 2023 Open Source Robotics Foundation

@mjcarroll
Copy link
Contributor Author

@ahcorde I think I addressed most of this feedback in #339 where I also split the commits a bit more logically. I'm going to close this PR in favor of that one.

@mjcarroll mjcarroll closed this May 18, 2023
@mjcarroll mjcarroll deleted the mjcarroll/rework-message-generation branch June 22, 2023 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants