NASA Challenge_[franklinselva]_[Restructure the contents of demos to be modular and scalable]

space-ros / demos

Various Space ROS demos.

Apache License 2.0

25 stars 41 forks source link

NASA Challenge_[franklinselva]_[Restructure the contents of demos to be modular and scalable] #31

Open franklinselva opened 2 months ago

franklinselva commented 2 months ago

Fixes space-ros/space-ros#178

Merge queue no. 1

This PR restructures the contents of the demo (from space-ros/simulations, space-ros/docker, space-ros/demos) under a single tree. The main changes include

Add canadarm_description package for simulation assets of canadarm2
Add curiosity_description package for simulation assets of curiosity mars rover
Add gazebo packages for canadarm and curiosity rover demos
Add docker based entrypoints for canadarm and curiosity rover demos
Add shell scripts for easier building and running of candarm and curiosity demos

This PR also restructure the workflow of the past demo in such a way the GUI apps are ran outside the docker. The reason for this change is to adapt different simulation tools and have a reproducible demo across the simulation tools.

franklinselva commented 2 months ago

Note for reviewers: This is a large refactoring related to the referenced issue. I think it would be easier to review the work from clear slate. If there is any information needed, please let me know.

mkhansenbot commented 2 months ago

Overall I like the changes, but I do have some concern about using a Makefile to build the Docker images. We're not doing that elsewhere. We've already got both Docker and Earthly in Space ROS, so adding a 3rd build method seems unnecessary. Would you consider moving to either a docker-compose or Earthly build instead?

mkhansenbot commented 2 months ago

Also, I'm planning to add a GH action build flow, see issue #35. If I get that in, I'm going to ask you to add build.sh files to wrap all the builds generically too.

franklinselva commented 2 months ago

Hi @mkhansenbot, Thanks for your review. I understand the use of Makefile as a runtime dependency is odd that is not in favor. I will make a direct replacement of the makefile with the shell script to build, clean and run the demos.

Would you consider moving to either a docker-compose or Earthly build instead?

I would not go for Earthly builds for demo since this will also be an addtional runtime dependency for working with the demos. At the moment, Earthly builds are only used as internal tool within collaborators and the scope of the build tool doesn't need to be expanded beyond this point. As mentioned here in the issue, using docker-compose for show casing different demos supported by shell scripts would be an good choice in my opinion. This will ensure, we still have the required entrypoints to the demos.

EDIT: I still see the space-ros docker images are expected to be pulled from registry and should not build when running the demos.

franklinselva commented 2 months ago

I have replaced the Makefiles with the build.sh. There are still few changes expected in the shell script based on #35. I will need to hold the rebasing on queued PRs until this PR is resolved.

EzraBrooks commented 2 months ago

Thanks for consolidating the simulation assets into one repository! This is a much more straightforward package layout.

franklinselva commented 1 month ago

@mkhansenbot Can you approve the changes for the CI run?

franklinselva commented 1 month ago

Is it possible to include docker-compose for the CI run?

mkhansenbot commented 1 month ago

@franklinselva - according to GitHub, Docker Compose v1 is no longer supported, can you change your script to use v2? https://github.blog/changelog/2024-04-10-github-hosted-runner-images-deprecation-notice-docker-compose-v1/