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

Migrate Package Specific Dependencies from Image to package.xml #244

Conversation

SPDonaghy
Copy link
Contributor

@SPDonaghy SPDonaghy commented Feb 23, 2024

Description

Some Python dependencies needed for the local pathfinding ROS package were moved from base-dev.Dockerfile to package.xml in the local_pathfinding repository

Verification

  • After removing some dependencies from base-dev.Dockerfile, I ran the build images workflow and set the new image tag in the main Dockerfile.
  • I then rebuilt the container and verified that the pip3 dependencies in base-dev.Dockerfile were installed in the workspace but the dependencies that were moved to package.xml were not.
  • Next, I ran the setup task and then verified the remaining dependencies were installed.

Resources

  • This PR only dealt with local_pathfinding dependencies hence only partially resolving the mentioned Sailbot Workspace issue.
  • Here is the related local_pathfinding PR

To Do

  • [Patrick] Cleanup dockerfile
  • [Sean] revert local pathfinding branch to main once its PR is merged
  • Migrate NET package dependencies to package.xml
    • "install base apt dependencies" section
    • "install some clang tools and googletest for network systems" section

@SPDonaghy
Copy link
Contributor Author

Colcon test failed because pyproj is not installed when the tests run. Maybe if we also specify pyproj as a test dependency in package.xml then it will work.

@patrick-5546
Copy link
Member

Setup is run in the tests run:

I think it is failing because it is pulling the main branch of local pathfinding. You can test out your branch by changing this line:

version: main

@SPDonaghy
Copy link
Contributor Author

I think it is failing because it is pulling the main branch of local pathfinding. You can test out your branch by changing this line:

Hm I tried this but its still not working.

This is what you meant right?
image

Running the setup task manually and then the test task works.
But just running test does not. It also doesn't complain if I put a wrong branch name in polaris.repos either.

@patrick-5546
Copy link
Member

patrick-5546 commented Feb 23, 2024

Hm I tried this but its still not working.

It timed out, which is a known bug: #231

After rerunning it passes

Copy link
Member

@patrick-5546 patrick-5546 left a comment

Choose a reason for hiding this comment

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

Can merge your local pathfinding pr in, change the branch back to main here, then merge this pr in

Copy link
Member

@patrick-5546 patrick-5546 left a comment

Choose a reason for hiding this comment

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

Actually let's keep this open for now and use it to clean up our dependencies across the software team. I'll add a to do list to this PR

@SPDonaghy
Copy link
Contributor Author

Actually let's keep this open for now and use it to clean up our dependencies across the software team. I'll add a to do list to this PR

Sounds good, and I'll make separate PRs in each repo to update the package.xml files one by one.

@patrick-5546
Copy link
Member

I'll make separate PRs in each repo to update the package.xml files one by one

As far as I can tell what's left is syncing with NET/diagnostics about what dependencies can be migrated to their repo

@SPDonaghy
Copy link
Contributor Author

After rerunning it passes

So on this branch, when you rebuild and then run the test task, it doesn't error out for you? I'm getting module not found for scipy from controller and pyproj from local_path

@SPDonaghy SPDonaghy requested a review from hhenry01 as a code owner March 18, 2024 19:03
.devcontainer/Dockerfile Outdated Show resolved Hide resolved
@SPDonaghy SPDonaghy enabled auto-merge (squash) March 19, 2024 23:30
@SPDonaghy SPDonaghy merged commit e0bdfc7 into main Mar 19, 2024
12 checks passed
@SPDonaghy SPDonaghy deleted the user/SPDonaghy/migrate-package-specific-dependencies-from-image-to-packages-242 branch March 19, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore ROS package dependencies
3 participants