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

Fix remaining issues in applications and demos from the updated spaceros base image #189

Merged
merged 6 commits into from
Oct 1, 2024

Conversation

eholum
Copy link
Contributor

@eholum eholum commented Sep 19, 2024

Resolves #161.

There are still issues building main for moveit and nav2 after migration of the base image build to the space-ros repo. This PR fixes them, including

  • Updating changed environment variable names
  • Fixes references to moved or removed directories in the base image
  • Parameterizes the base image name for building locally and in CI

As the base main image is now pushed to GHCR, we should use that when building in CI. The change can be tested locally if you have access to the container registry (you must be logged in):

Update:

Depends on space-ros/space-ros#220

Because the GHCR images are not public, we can temporarily point to the main branch in the docker registry. This can be tested with either moveit2 or nav2 with:

$ docker pull osrf/space-ros:main
$ SPACE_ROS_IMAGE=osrf/space-ros:main ./build.sh

@asimonov
Copy link

I am trying to test #152, but that requires moveit2 docker which is failing on main.

shall we test/push this one first to make life easier?

@eholum
Copy link
Contributor Author

eholum commented Sep 24, 2024

@asimonov this should fix the nav2 build if you have a locally built space ros image. Still working on getting main pushed somewhere that we can actually use it in space-ros/space-ros#220.

But yes for testing #152 you could just manually change the name of the starting image to use a locally built version of space-ros:main from your machine.

@eholum eholum marked this pull request as ready for review September 24, 2024 16:45
@asimonov
Copy link

yes, I am using the locally built images. but it is getting confusing...

basically if our ultimate test is to run moveit and rover demos that requires all the images to 1. be built, 2. be consistent. and they require different PRs to be brought to that state.

if we could save branch tagged images and start merging PRs, that would alleviate confusion to some degree

@eholum
Copy link
Contributor Author

eholum commented Sep 27, 2024

yes, I am using the locally built images. but it is getting confusing...

Ugh, yes sorry things have been in an odd state, but now that the main image is updated and pushed to Dockerhub, too things should be more clear? This PR should resolve the build issues on main, and also open things up to using different base images for the moveit and nav2 builds.

@asimonov
Copy link

yes, I am using the locally built images. but it is getting confusing...

Ugh, yes sorry things have been in an odd state, but now that the main image is updated and pushed to Dockerhub, too things should be more clear? This PR should resolve the build issues on main, and also open things up to using different base images for the moveit and nav2 builds.

almost... git-lfs change on simulation broke my local build of docker/space_robots... space-ros/simulation#35

i think we need to test every PR on affected repos by running those 2 demos. because they effectively combine everything we have

@asimonov
Copy link

Erik, can you include the lfs setup into this PR as well?

@eholum
Copy link
Contributor Author

eholum commented Sep 29, 2024

Erik, can you include the lfs setup into this PR as well?

Ugh, in the interest of traceability I included that fix in #194, since that was broken by a separate change...

i think we need to test every PR on affected repos by running those 2 demos. because they effectively combine everything we have

This is a big ask until we get some sort of automated infrastructure in place to at least pre-build those images. Or in an ideal worth a pre-build and a basic sanity check. But hopefully we can get that done sooner rather than later?

Copy link

@asimonov asimonov left a comment

Choose a reason for hiding this comment

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

looks good. and it builds locally.

but i cannot test demos (mars rover and navigation) because of other issues (e.g. #194)

i suggest we merge all these PRs releated to fixing the build and then re-test the demos

Copy link
Member

@EzraBrooks EzraBrooks left a comment

Choose a reason for hiding this comment

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

Can we break out the GitHub Actions version bumps into another issue/PR? Everything else is clearly related

@eholum
Copy link
Contributor Author

eholum commented Oct 1, 2024

Can we break out the GitHub Actions version bumps into another issue/PR? Everything else is clearly related

Done. Squashed those changes out of the param change commits and updated. Everything else remains unchanged. I'll open a separate issue to update CI since there's more that would be helpful now that we have main available...

@eholum eholum requested a review from EzraBrooks October 1, 2024 17:03
Copy link
Member

@EzraBrooks EzraBrooks left a comment

Choose a reason for hiding this comment

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

These code changes all make obvious sense to me. (which I say in lieu of testing)

@eholum eholum merged commit 09ec444 into main Oct 1, 2024
4 checks passed
@eholum
Copy link
Contributor Author

eholum commented Oct 1, 2024

i suggest we merge all these PRs releated to fixing the build and then re-test the demos

The time is nigh!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Adjust READMEs and Apply Fixes for Demos Base Images
3 participants