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

Make ament_cmake_python symlink for symlink installs only #357

Merged
merged 1 commit into from
Oct 13, 2021

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Oct 7, 2021

Precisely what the title says. Otherwise, #350 hits our Windows users.

@hidmic
Copy link
Contributor Author

hidmic commented Oct 7, 2021

@gavanderhoorn give this one a shot and let me know.

Copy link

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Seems to have fixed it.

Tested on Win10 + Python 3.8.x.

@gavanderhoorn
Copy link

I'll test it some more, but it seems like this has resolved the immediate issue.

Thanks for the fast turn-around @hidmic 👍

@gavanderhoorn
Copy link

Will this be backported to galactic? Seeing as it fixes something which seems like a regression?

@hidmic
Copy link
Contributor Author

hidmic commented Oct 7, 2021

Full CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Contributor Author

hidmic commented Oct 7, 2021

Will this be backported to galactic? Seeing as it fixes something which seems like a regression?

I'm onboard with that. @cottsay ?

@gavanderhoorn
Copy link

Friendly ping?

@hidmic
Copy link
Contributor Author

hidmic commented Oct 13, 2021

I was out for a few days @gavanderhoorn. Sorry for the delay.

Hmm, those rclpy test failures on Windows look suspicious.

Running Windows CI up to rclpy: Build Status

@gavanderhoorn
Copy link

might be flaky tests?

@clalancette
Copy link
Contributor

might be flaky tests?

They were fixed elsewhere in the stack in the meantime.

@hidmic
Copy link
Contributor Author

hidmic commented Oct 13, 2021

Alright then. Going in!

@hidmic hidmic merged commit f69df01 into master Oct 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the hidmic/symlink-on-symlink-installs-only branch October 13, 2021 18:16
@gavanderhoorn
Copy link

Thanks for fixing this @hidmic 👍

@gavanderhoorn
Copy link

Oh. And the backport to Galactic?

@hidmic
Copy link
Contributor Author

hidmic commented Oct 13, 2021

Oh. And the backport to Galactic?

You can cherry pick the commit and open a backport PR. Otherwise, I'll try to do it later today.

Edit: I wonder if we can fast-forward.

gavanderhoorn pushed a commit to Yaskawa-Global/ament_cmake that referenced this pull request Oct 13, 2021
@gavanderhoorn
Copy link

I can create the PR (I believe).

Not sure whether ff would work. Are all those commits supposed to end up on galactic as well?

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