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

update install_deps for v2x-ros-conversion #256

Open
wants to merge 3 commits into
base: develop-humble
Choose a base branch
from

Conversation

adev4a
Copy link
Contributor

@adev4a adev4a commented Feb 14, 2025

PR Details

This PR updates the installation for the stol-j2735 library to use the correct version based on branch and ubuntu distro.

Description

Related GitHub Issue

Related Jira Key

Motivation and Context

How Has This Been Tested?

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@adev4a
Copy link
Contributor Author

adev4a commented Feb 15, 2025

This PR requires usdot-fhwa-stol/v2x-ros-conversion#12 to be merged first.

Copy link
Contributor

@paulbourelly999 paulbourelly999 left a comment

Choose a reason for hiding this comment

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

Instead of using the install_dependencies.sh script of v2x-ros-conversion I think it may be better to setup our own install_dependencies.sh script for this repo. My thought process is the script only installs a single package currently. If in the future carma-messenger uses more apt packages it seems that it should have it's own script to install them. What do you think?

@paulbourelly999 paulbourelly999 self-requested a review February 18, 2025 15:18
Copy link
Contributor

@paulbourelly999 paulbourelly999 left a comment

Choose a reason for hiding this comment

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

See comment

@adev4a
Copy link
Contributor Author

adev4a commented Feb 19, 2025

Instead of using the install_dependencies.sh script of v2x-ros-conversion I think it may be better to setup our own install_dependencies.sh script for this repo. My thought process is the script only installs a single package currently. If in the future carma-messenger uses more apt packages it seems that it should have it's own script to install them. What do you think?

Makes sense to me, specially as we are migrating packages to debian, sounds like a good idea to keep an install deps script.

shift
shift
;;
-r|--root)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove root argument since it is not used


# Install dependencies
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
${SCRIPT_DIR}/install_dependencies.sh -b $BRANCH -r $dir
Copy link
Contributor

Choose a reason for hiding this comment

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

remove -r argument since it is not used



# Install dependencies
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I don't think you need this. I think you should just be able to call

./install_dependencies.sh ....

Copy link
Contributor

@paulbourelly999 paulbourelly999 left a comment

Choose a reason for hiding this comment

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

small changes

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