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

Mark optional dependencies in package.xml. #376

Merged
merged 4 commits into from
Apr 22, 2018

Conversation

brianhou
Copy link
Contributor

@brianhou brianhou commented Apr 17, 2018

Depends on personalrobotics/pr-cleanroom#14.

In the CI scripts, we should now run something like

./scripts/internal-distro.py ... -o OPTIONAL

where OPTIONAL is the name of the dependency group below.


Before creating a pull request

  • Document new methods and classes
  • Format code with make format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@brianhou brianhou requested a review from jslee02 April 17, 2018 22:49
jslee02
jslee02 previously approved these changes Apr 17, 2018
Copy link
Member

@jslee02 jslee02 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 to me.

Once this PR is merged, I'll update #373 reflecting this change and personalrobotics/pr-cleanroom#14.

@codecov
Copy link

codecov bot commented Apr 18, 2018

Codecov Report

Merging #376 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
- Coverage   83.48%   83.46%   -0.02%     
==========================================
  Files         197      197              
  Lines        5619     5619              
==========================================
- Hits         4691     4690       -1     
- Misses        928      929       +1
Impacted Files Coverage Δ
src/planner/ompl/CRRT.cpp 75.58% <0%> (-0.59%) ⬇️

Copy link
Contributor

@gilwoolee gilwoolee 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 update the readme and say that these needs to be uncommented for using some of of the underlying packages?

@aditya-vk
Copy link
Contributor

Looks like libherb and herb3_demos won't pass any CI tests until the optional dependencies issue isn't fixed. We should probably close this with a neat solution [force all optional dependencies to be installed?] soon before merging any PRs in the other downstream packages?

@brianhou
Copy link
Contributor Author

brianhou commented Apr 22, 2018

Here's another proposal for managing optional dependencies:

  • By default, all dependencies (including optional dependencies) are required.
  • Introduce <optional> tags (inside <export>) to mark which of the <depend>s are optional. We should be able to programmatically access those in the pr-cleanroom/internal-distro.py script and remove them from the set of dependencies (based on some CLI flag). We'll only do this when testing on Travis.

I've updated package.xml to match this proposal, and will update the internal-distro.py script next.

Edit: depends on personalrobotics/pr-cleanroom#16.

@jslee02 jslee02 mentioned this pull request Apr 22, 2018
1 task
@jslee02 jslee02 merged commit 5cc94e7 into master Apr 22, 2018
@jslee02 jslee02 deleted the enhancement/brianhou/optional-deps branch April 22, 2018 03:12
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.

4 participants