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

colcon plugin: support build-time chaining #2486

Merged
merged 2 commits into from
Feb 26, 2019

Conversation

kyrofa
Copy link
Contributor

@kyrofa kyrofa commented Feb 25, 2019

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh tests/unit?

Until recently, colcon had a bug that caused it to not chain workspaces at build-time. That was recently fixed in a way that broke the colcon plugin. This PR fixes LP: #1816565 by reworking the plugin to rely more on the setup files and less on environment variables to avoid this issue in the future.

Until recently, colcon had a bug that caused it to not chain workspaces
at build-time. That was recently fixed in a way that broke the colcon
plugin. Rework the plugin to rely more on the setup files and less on
environment variables to avoid this issue in the future.

LP: #1816565

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
Copy link
Collaborator

@sergiusens sergiusens left a comment

Choose a reason for hiding this comment

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

Looks ok, would be nice to see a run after the unused imports are removed.

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@53f96dc). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2486   +/-   ##
=========================================
  Coverage          ?   89.88%           
=========================================
  Files             ?      198           
  Lines             ?    13339           
  Branches          ?     2021           
=========================================
  Hits              ?    11990           
  Misses            ?      924           
  Partials          ?      425
Impacted Files Coverage Δ
snapcraft/plugins/colcon.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53f96dc...2cfab7c. Read the comment docs.

# overall, but it defines it after this function runs. Some ROS
# tools will cause binaries to be run when we source the setup.sh,
# below, so we need to have a sensible LD_LIBRARY_PATH before then.
'LD_LIBRARY_PATH="$LD_LIBRARY_PATH:{}"'.format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good to see this go away

Copy link
Collaborator

@sergiusens sergiusens left a comment

Choose a reason for hiding this comment

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

This is one complex puzzle!

@sergiusens sergiusens merged commit 60c3c9b into canonical:master Feb 26, 2019
@kyrofa kyrofa deleted the bugfix/1816565/colcon_chaining branch February 26, 2019 17:54
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.

3 participants