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

source bash completion script from setup file #84

Merged
merged 2 commits into from
Mar 27, 2018

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Mar 26, 2018

Fixes #78. Connect to #78.

I have applied the same changes to the installed location in /opt/ros/ardent which made it work for me from Debian packages when sourcing /opt/ros/ardent/local_setup.bash.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Mar 26, 2018
@dirk-thomas dirk-thomas self-assigned this Mar 26, 2018
@mikaelarguedas
Copy link
Member

Will this work only for bash and not other shells like zsh or sh ?

@dirk-thomas
Copy link
Member Author

sh doesn't have completion functionality and there is no zsh specific prefix-level script at the moment.

@mikaelarguedas
Copy link
Member

and there is no zsh specific prefix-level script at the moment

I'm not sure I understand that bit. Can you clarify?

Is the conclusion is that zsh users will need to source ros2-argcomplete.zsh manually?

@dirk-thomas
Copy link
Member Author

Can you clarify?

I guess my install directory doesn't have a .zsh file since I am not building with ament_tools. I will add local_setup.zsh file to this PR so also automate this for zsh.

@dirk-thomas
Copy link
Member Author

See a151f89 for a zsh specific local setup file.

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm, didn't try the zsh one though as I use bash

@dirk-thomas dirk-thomas merged commit f33435c into master Mar 27, 2018
@dirk-thomas dirk-thomas deleted the source_completion_from_setup branch March 27, 2018 21:30
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Mar 27, 2018
@dirk-thomas
Copy link
Member Author

I propose to cherry-pick this change to the ardent branch and release this repo into ardent. That will allow us to confirm that it actually works in the Debian case (and the users will benefit from it early). Any thoughts on this?

@wjwwood
Copy link
Member

wjwwood commented Mar 27, 2018

You have to not only cherry-pick to the ardent branch but also make a release to see if it works in the debian case right?

I'm not opposed to it either way, but I would prefer to not have any kind of partial re-release of ardent. One related concern is that someone getting the ardent branch would not be getting what is in the binaries, but then again they should probably use tags instead if they want exactly what's in binaries.

@mikaelarguedas
Copy link
Member

I propose to cherry-pick this change to the ardent branch and release this repo into ardent. That will allow us to confirm that it actually works in the Debian case (and the users will benefit from it early). Any thoughts on this?

Does this imply rebuilding the artifacts on all platforms to have consistent release state everywhere ?

@dirk-thomas
Copy link
Member Author

You have to not only cherry-pick to the ardent branch but also make a release to see if it works in the debian case right?

Yes, "cherry-pick this change to the ardent branch and release this repo into ardent".

One related concern is that someone getting the ardent branch would not be getting what is in the binaries

Does this imply rebuilding the artifacts on all platforms to have consistent release state everywhere ?

I don't think this is necessary. This problem is specific to our Debian packages. The fat archives as well as a from-source build are already working (since ament_tools is taking care of sourcing the completion script). Therefore I would not release new fat archives.

@mikaelarguedas
Copy link
Member

I don't think this is necessary. This problem is specific to our Debian packages. The fat archives as well as a from-source build are already working (since ament_tools is taking care of sourcing the completion script). Therefore I would not release new fat archives.

I'm fine with it as no actual code is touched, ti will make the behavior consistent across the board and the release overhead will be minimal

dirk-thomas added a commit that referenced this pull request Mar 28, 2018
* source bash completion script from setup file

* add zsh specific local_setup file
@dirk-thomas
Copy link
Member Author

Released 0.4.1: ros2/rosdistro#77

ruffsl added a commit to keymint/keymint_cli that referenced this pull request Jul 24, 2018
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.

None yet

3 participants