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

Install catkin-pkg on OS X using PIP #286

Merged
merged 1 commit into from
Oct 14, 2020
Merged

Conversation

thomas-moulard
Copy link
Member

Description

  • Does this PR check in generated JS code (npm run build)?

@thomas-moulard thomas-moulard requested a review from a team as a code owner October 14, 2020 15:44
@thomas-moulard thomas-moulard requested review from zmichaels11 and prajakta-gokhale and removed request for a team October 14, 2020 15:44
@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #286 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #286   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files           8        8           
  Lines         151      151           
  Branches       10       10           
=======================================
  Hits          141      141           
  Misses         10       10           
Impacted Files Coverage Δ
src/setup-ros-osx.ts 100.00% <100.00%> (ø)

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 736bf6f...cacd2a3. Read the comment docs.

@thomas-moulard thomas-moulard force-pushed the tmoulard/pip_catkin_osx branch from 62bf9a6 to 6af93e9 Compare October 14, 2020 15:57
Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
@thomas-moulard thomas-moulard force-pushed the tmoulard/pip_catkin_osx branch from 6af93e9 to cacd2a3 Compare October 14, 2020 16:18
@thomas-moulard
Copy link
Member Author

This fixes the action-ros-ci build failures on OS X: ros-tooling/action-ros-ci#387

Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

This seems fine, but why did builds work before and not now?

@@ -5962,7 +6027,7 @@ function runOsX() {
yield pip.installPython3Dependencies();
// While rosdep and vcs are available as a Debian package on Ubuntu, they need
// to be installed through pip on OS X.
yield pip.runPython3PipInstall(["rosdep", "vcstool"]);
yield pip.runPython3PipInstall(["catkin-pkg", "rosdep", "vcstool"]);
Copy link

@jaisontj jaisontj Oct 14, 2020

Choose a reason for hiding this comment

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

I am not entirely sure about this, but isnt this built automatically by npm run build or its equivalent? Shouldnt the change in src/setup-ros-osx.ts be enough?

Copy link
Member Author

@thomas-moulard thomas-moulard Oct 14, 2020

Choose a reason for hiding this comment

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

No, we need to commit the generated file too.

@@ -27,7 +27,7 @@ export async function runOsX() {

// While rosdep and vcs are available as a Debian package on Ubuntu, they need
// to be installed through pip on OS X.
await pip.runPython3PipInstall(["rosdep", "vcstool"]);
await pip.runPython3PipInstall(["catkin-pkg", "rosdep", "vcstool"]);

Choose a reason for hiding this comment

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

Looks like this install was missing, but I'm confused as to how this was not failing earlier?

@thomas-moulard
Copy link
Member Author

I don't know why it started failing but DIrks' answer to someone with a similar issue is that it's working as intended and that installing catkin-pkg is mentioned in the doc: ros2/ros2#503

@thomas-moulard thomas-moulard merged commit 0b28c3a into master Oct 14, 2020
@thomas-moulard thomas-moulard deleted the tmoulard/pip_catkin_osx branch October 14, 2020 16:37
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