Skip to content

SROS2 cmake restructure#74

Merged
ruffsl merged 1 commit intoros2:masterfrom
ross-desmond:ros2_cmake_restructure
Jan 11, 2019
Merged

SROS2 cmake restructure#74
ruffsl merged 1 commit intoros2:masterfrom
ross-desmond:ros2_cmake_restructure

Conversation

@ross-desmond
Copy link
Copy Markdown
Contributor

Restructures sros2 to the following: @jacobperron

Just a reminder the security_helpers folder in sros2 may want to be split out to another package as it does not build under the current sros2 package structure. If we believe this helper macro should stay in sros2 then I'll need to restructure to build sros2 python modules and the helper macro, thoughts?

IMO, the cmake package can remain in this repo (but remain a separate package) as it matches the repo description. However, I would consider remaining the package to something like sros2_cmake, since its main purpose is providing cmake macros for interacting with ROS 2 security.
I would restructure this repo so that the two packages are at top along with the general docs about sros2:

sros2
  |_ sros2 (python lib)
  |_ test
  |_ package.xml
  |_ setup.py
  |_ ... etc
sros2_cmake
  |_ cmake
    |_ ros2_create_keystore.cmake
    |_ ros2_secure_node.cmake
  |_ CMakeLists.txt
  |_ package.xml
  |_ ... etc
CONTRIBUTING.md
LICENSE
README.md
SROS2_Linux.md
SROS2_MacOS.md
SROS2_Windows.md

I'd like to get some other thoughts on this proposal.

If we go this approach, I think it would be better if the cmake package is added in a separate PR as it is distinct from the CLI changes and is a larger change.

Depends on #71

@tfoote tfoote added the in review Waiting for review (Kanban column) label Jan 11, 2019
@ruffsl
Copy link
Copy Markdown
Member

ruffsl commented Jan 11, 2019

Could you separate the migration of the sros2 python lib folder rename/restructure vs the addition of the sros2_cmake stuff into seperate PRs? Then I could work on rebasing my xml PR sooner while the cmake stuff is still in review.

Enables addition of new colcon built packages in this repo
@ross-desmond ross-desmond force-pushed the ros2_cmake_restructure branch from 5ef8fab to d918801 Compare January 11, 2019 18:49
@ross-desmond ross-desmond mentioned this pull request Jan 11, 2019
Copy link
Copy Markdown
Member

@ruffsl ruffsl left a comment

Choose a reason for hiding this comment

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

LGTM

@ruffsl ruffsl merged commit 0ace86d into ros2:master Jan 11, 2019
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jan 14, 2019
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.

4 participants