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

changes submodules to debians #76

Merged
merged 41 commits into from
May 29, 2024

Conversation

xwilson03
Copy link
Collaborator

@xwilson03 xwilson03 commented May 23, 2024

PR Summary

PR Link: Link

Issue Link: Link

Description

Switches compilation-heavy submodules for pre-compiled debians to assist CI pipeline and embedded devices.

Reviewers


Changelog

  • Switches submodules to track VEXU_GHOST forks.
  • Changes setup_submodules.sh script to pull debians from new ghost_dependencies repository instead of compiling from source locally.
  • Skips debianized submodules in build and test scripts.
  • Skips building visualization packages on arm machines (jetson).
  • Adds reverse-ssh tunnel to CI pipeline for post-failure debugging.

@xwilson03 xwilson03 requested a review from MaxxWilson May 23, 2024 09:24
@xwilson03 xwilson03 self-assigned this May 23, 2024
@MaxxWilson
Copy link
Collaborator

MaxxWilson commented May 24, 2024

Looks pretty solid. Left relevant comments.

Another big question, do we know how this works for ROS Package dependencies? i.e. exporting the proper environment variables so that the ros packages are detected? None of these submodules are ros packages, and BehaviorTree related ones seem to take a bit.

I think should be good to merge AFTER you figure out the Jetson binaries or make an issue to do it later. Im approving for now because I dont have bandwidth to double check later, but I assume you will handle these before merging.

@MaxxWilson
Copy link
Collaborator

MaxxWilson commented May 24, 2024

FOLLOWUP:

#77 This needs to get merged into this branch so we can debianize plotjuggler and plotjuggler_ros bc they are THICK and effect every build. We should also just do the cpp and lidar ones while we have this PR open, bc otherwise we never will lol.

  • Debianize plotjuggler
  • Debianize plotjuggler-ros
  • Debianize rplidar
  • Debianize BehaviorTreeROS2
  • Debianize BehaviorTreeCpp
  • Add arm64 debians
  • Error checking on $VEXU_HOME

@xwilson03
Copy link
Collaborator Author

xwilson03 commented May 24, 2024

Looks pretty solid. Left relevant comments.

Another big question, do we know how this works for ROS Package dependencies? i.e. exporting the proper environment variables so that the ros packages are detected? None of these submodules are ros packages, and BehaviorTree related ones seem to take a bit.

I think should be good to merge AFTER you figure out the Jetson binaries or make an issue to do it later. Im approving for now because I dont have bandwidth to double check later, but I assume you will handle these before merging.

@MaxxWilson This should be possible through pre/post installation scripts.
Since the submodule debians are somewhat hand-crafted, maintainers would need to ensure that the proper environment variables get set post-install.

EDIT: do not use post install scripts in the way that i did! use bloom to handle this instead of trying to do it yourself: https://docs.ros.org/en/rolling/How-To-Guides/Building-a-Custom-Debian-Package.html

@xwilson03
Copy link
Collaborator Author

All x86 debians have been added, just not their arm equivalents.
Moving onto a small testing phase using the pipeline to ensure their quality; the build script will need to omit these packages going forward.

@xwilson03
Copy link
Collaborator Author

xwilson03 commented May 28, 2024

update on this; regular submodules continue to work flawlessly, but the mentioned approach to packaging ROS dependencies (postinst scripts that move files into the repository directory using $VEXU_HOME) was super hacky and now needs to be pretty much overhauled and replaced with bloom generated packages.

@xwilson03
Copy link
Collaborator Author

tests for submodules got disabled since the submodules themselves werent technically "built"; some file location assertions were failing and necessitated this. either we need to skip submodule tests (current solution) or we figure out a way around this somehow.

other than that, though, pipeline is passing so this is ready for a merge. arm64 binaries are otw via the ghost_dependencies repo, but all the work here is done.

@xwilson03 xwilson03 merged commit 77e483a into develop May 29, 2024
1 check passed
@xwilson03 xwilson03 deleted the feature/changes-submodules-to-debians branch May 29, 2024 23:20
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.

2 participants