Skip to content

Initial ros2 cookbooks for Windows#3

Merged
brawner merged 9 commits intomasterfrom
brawner/ros2-windows
May 15, 2020
Merged

Initial ros2 cookbooks for Windows#3
brawner merged 9 commits intomasterfrom
brawner/ros2-windows

Conversation

@brawner
Copy link
Copy Markdown
Collaborator

@brawner brawner commented Apr 21, 2020

This is the initial PR for ROS 2 cookbooks on Windows. For ease of review, the vendor cookbooks were separated out. The new content is added in this commit: c091be0

@brawner brawner requested a review from nuclearsandwich April 21, 2020 16:59
@brawner
Copy link
Copy Markdown
Collaborator Author

brawner commented Apr 24, 2020

Third party dependency cookbooks have been removed in favor of berkshelf file.

nuclearsandwich
nuclearsandwich reviewed Apr 28, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

I did an initial pass.

There are a few cleanup items adapting a cookbooks repo from a full chef repository. Several nitpicks. And some organization items like using different recipes rather than including recipes conditionally based on attributes.

The asserts in the rti recipe give me pause. I want to come back and do a closer review of that file.

Comment thread .chef-repo.txt Outdated
Comment thread .chef/solo.rb Outdated
Comment thread chefignore
Comment thread cookbooks/ros2_windows/README.md Outdated
Comment thread cookbooks/ros2_windows/recipes/chocolatey_installs.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/visual_studio.rb
Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/ros2.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/ros2.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
nuclearsandwich
nuclearsandwich reviewed Apr 28, 2020
View reviewed changes
Comment thread cookbooks/ros2_windows/recipes/pip_installs.rb Outdated
@brawner brawner force-pushed the brawner/ros2-windows branch from aba05e5 to 91f2bd3 Compare April 28, 2020 22:44
brawner
brawner commented Apr 28, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator Author

@brawner brawner left a comment

Choose a reason for hiding this comment

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

Thanks @nuclearsandwich for the review. I believe I've addressed your feedback. I launched another test over at ros2/ci#425

Comment thread .chef-repo.txt Outdated
Comment thread .chef/solo.rb Outdated
Comment thread chefignore
Comment thread cookbooks/ros2_windows/README.md Outdated
Comment thread cookbooks/ros2_windows/recipes/chocolatey_installs.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/ros2.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/visual_studio.rb
Comment thread cookbooks/ros2_windows/recipes/qt5.rb Outdated
nuclearsandwich
nuclearsandwich requested changes Apr 29, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

Did some investigation about error handling in recipes. I think we should remove the asserts in the rti_connext recipe.

Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated

assert_not_nil(connext_params['min_vs_version'], "Minimum Visual Studio version is required (e.g. '2017')")

assert_not_nil(connext_params['openssl_version'], "OpenSSL version is required (e.g. '1.0.2n')")
Copy link
Copy Markdown
Contributor

@nuclearsandwich nuclearsandwich Apr 29, 2020

Choose a reason for hiding this comment

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

Okay I think what we want to do here is use the Chef logging facility to log fatal errors and then raise an error. The log resource has some examples: https://docs.chef.io/resources/log/

nuclearsandwich
nuclearsandwich approved these changes Apr 29, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@nuclearsandwich nuclearsandwich left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A couple of nitpicks but has really shaped up!

Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
brawner
brawner commented Apr 30, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator Author

@brawner brawner left a comment

Choose a reason for hiding this comment

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

Ok, fixed the if statements and running a new ci build.

Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
Comment thread cookbooks/ros2_windows/recipes/rti_connext.rb Outdated
@brawner brawner force-pushed the brawner/ros2-windows branch 6 times, most recently from add1b63 to 5f79bae Compare April 30, 2020 00:50
@brawner brawner force-pushed the brawner/ros2-windows branch 7 times, most recently from edc8df1 to 17cf6d7 Compare May 4, 2020 18:37
@brawner brawner force-pushed the brawner/ros2-windows branch 8 times, most recently from bbad0a1 to dfb190e Compare May 5, 2020 00:30
@brawner brawner force-pushed the brawner/ros2-windows branch from dfb190e to 76e4f3d Compare May 5, 2020 17:51
@brawner
Copy link
Copy Markdown
Collaborator Author

brawner commented May 5, 2020

Latest CI job builds successfully!

Build Status

@brawner brawner merged commit 0bbc506 into master May 15, 2020
@nuclearsandwich nuclearsandwich deleted the brawner/ros2-windows branch May 15, 2020 18:32
@brawner brawner mentioned this pull request Jul 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nuclearsandwich nuclearsandwich nuclearsandwich approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@brawner @nuclearsandwich