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

add colcon_cd function #1

Merged
merged 3 commits into from
Oct 11, 2019
Merged

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Sep 30, 2019

  • colcon_cd <pkgname> will change the current working directory to the directory the specified package was found (usually in the source space)
    • if multiple locations are found they are all printed and the current working directory is switched to the first one
  • colcon_cd changes the current working directory back to where the first command was called initially (usually the root of a workspace)

Requires colcon/colcon-core#240 and colcon/colcon-output#23.

@dirk-thomas dirk-thomas added the enhancement New feature or request label Sep 30, 2019
@dirk-thomas dirk-thomas self-assigned this Sep 30, 2019
Copy link

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

A behavior of always preferring the workspace related to the current working directory seems more intuitive to me. And then falling back to a previous workspace if there was one.
But I'm not sure if there's a way to get the root of a colcon workspace from any subdirectory...

@dirk-thomas
Copy link
Member Author

dirk-thomas commented Sep 30, 2019

A behavior of always preferring the workspace related to the current working directory seems more intuitive to me.

Atm there is no way to determine a workspace root. See colcon/colcon-core#139 which is requiring a similar functionality.

And then falling back to a previous workspace if there was one.

For another underlay the only information about it is in the COLCON/AMENT_PREFIX_PATH which points to the install prefix which doesn't provide a way to derive the source directories of the packages in a reliable (non-heuristic) way.

The advantage of not requiring a workspace at all is that you can use the command anywhere in your file system.

@deitry
Copy link

deitry commented Oct 4, 2019

Why this is not implemented as colcon subverb, like colcon cd? I respect colcon because there is no commands with underscores as it was with catkin_make and so on. Adding one will lead to unnecessary diversity and confusion.

@dirk-thomas
Copy link
Member Author

Why this is not implemented as colcon subverb, like colcon cd?

@deitry Because a (Python) subprocess can't change the working directory of a shell. Therefore this functionality must be implemented as a shell function.

Regarding the naming of the function: in sh it is invalid to have dashes in function names.

Copy link

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

IIUC, the assumed workflow is the command is initially called from within a workspace, or requires a colcon_cd --set if changing workspaces.

In the scenario where multiple copies of a package exist in a directory tree, the command fails. For example, if my directory tree is

.
├── ws1
│   └── src
│       └── foo
│           ├── CMakeLists.txt
│           ├── include
│           │   └── foo
│           ├── package.xml
│           └── src
└── ws2
    └── src
        └── foo
            ├── CMakeLists.txt
            ├── include
            │   └── foo
            ├── package.xml
            └── src

And I run colcon_cd foo, I get the following error:

Saved the directory '/home/jacob/ws/test_colcon_cd' for future invocations of 'colcon_cd <pkgname>' as well as to return to using  'colcon_cd'. Call 'colcon_cd --reset' to unset the saved path.
bash: cd: $'/home/jacob/ws/test_colcon_cd/ws1/src/foo\n/home/jacob/ws/test_colcon_cd/ws2/src/foo': No such file or directory

I think we could use a better error message about discovering multiple packages with the same name, and consider a fall-back behaviour to cd to the first instance.

I also can't use colcon_cd with ZSH, e.g.

$ colcon_cd rclpy
Could not find package 'rclpy' from the current working directory

And after trying colcon_cd --set:

Could neither find package 'rclpy' from '/home/jacob/ws/latest_ws' nor from the current working directory

@dirk-thomas
Copy link
Member Author

I think we could use a better error message about discovering multiple packages with the same name, and consider a fall-back behaviour to cd to the first instance.

Yeah, it should definitely handle duplicate package names - done in: 2244869.

@dirk-thomas
Copy link
Member Author

I also can't use colcon_cd with ZSH

Fixes in c9c66e8.

Copy link

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

@dirk-thomas dirk-thomas merged commit fff5af2 into master Oct 11, 2019
@delete-merged-branch delete-merged-branch bot deleted the dirk-thomas/colcon_cd-function branch October 11, 2019 03:27
@dirk-thomas dirk-thomas added this to the 0.1.0 milestone Oct 11, 2019
@cottsay
Copy link
Member

cottsay commented Oct 11, 2019

Requires colcon/colcon-core#240 and colcon/colcon-output#23.

Should this be reflected in setup.cfg? Or is a more of a soft requirement?

@dirk-thomas
Copy link
Member Author

It doesn't strictly require those. If you have an older version it will just ignore the env var and generate a log directory`.

I will see if I add a version dependency anyway before releasing this... Thanks for the pointer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants