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

verbs: adding locate verb to get workspace paths #165

Closed
wants to merge 1 commit into from

Conversation

jbohren
Copy link
Contributor

@jbohren jbohren commented Mar 18, 2015

@k-okada

With this PR, calling this shell function from within a directory will source its devel space:

function source_devel() {
  source $(catkin locate --devel)/setup.bash
}
function source_devel() {
  source $(catkin locate --devel)/setup.zsh
}

cd to a package in the workspace, regardless of build_type (see #143)

function ccd() {
  cd $(catkin locate --src $1)
}

@k-okada
Copy link
Contributor

k-okada commented Mar 18, 2015

thanks, this is perfect!

@wjwwood
Copy link
Member

wjwwood commented Mar 18, 2015

I'd suggest a trip-wire test for this verb, just to catch things like that.

Otherwise, lgtm.

@@ -0,0 +1,24 @@
# Copyright 2014 Open Source Robotics Foundation, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: New files should get a 2015 copyright year.

@jbohren jbohren changed the title verbs: adding path verb to get workspace paths verbs: adding find verb to get workspace paths Mar 19, 2015
@jbohren
Copy link
Contributor Author

jbohren commented Mar 19, 2015

@k-okada @wjwwood I renamed the verb to catkin find and added an option to get the path to a workspace package. This also addresses usability concerns from #143.

# This describes this command to the loader
description = dict(
verb='find',
description="Get the paths to important locations in a workspace.",
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be update to include finding packages? I think it might work for both cases, though it is a little vague.

@wjwwood
Copy link
Member

wjwwood commented Mar 19, 2015

This also addresses usability concerns from #143.

By this I assume you mean it is addressed by using catkin find --pkg <package name> instead of roscd <package name>?


How does this compare or relate to catkin_find?

@jbohren
Copy link
Contributor Author

jbohren commented Mar 19, 2015

This also addresses usability concerns from #143.
By this I assume you mean it is addressed by using catkin find --pkg instead of roscd ?

Yeah, specifically cd $(catkin find --pkg <package name>)

This will work from within a workspace in the same way catkin build <package name> will.


How does this compare or relate to catkin_find?

This is currently very different from catkin_find, which is used to find resources related to various projects. This is more like catkin_find_package and catkin find --pkg <package name> is the same as catkin_find_package <package name> /path/to/ws.


Another interface I was considering would have a form like:

catkin find pkg <package name>`
catkin find src
catkin find devel
catkin find install

@wjwwood
Copy link
Member

wjwwood commented Mar 20, 2015

Hmm, the find implies searching, but really it's a look up (finding a package does crawl, but in a limited scope).

I don't necessarily have a problem with the verb being called find, but maybe we should take a moment and consider the alternatives.

  • catkin find
  • catkin path
  • catkin dir
  • catkin loc[ation]
  • catkin where[_is]

Others?


For reference brew has some -- options on the main command which provide similar info:

--cache
Display Homebrew's download cache. See also HOMEBREW_CACHE.

--cache formula
Display the file or directory used to cache formula.

--cellar
Display Homebrew's Cellar path. Default: /usr/local/Cellar

--cellar formula
Display the location in the cellar where formula would be installed, without any sort of versioned directory as the last path.

--env Show a summary of the Homebrew build environment.

--prefix
Display Homebrew's install path. Default: /usr/local

--prefix formula
Display the location in the cellar where formula is or would be installed.

--repository
Display where Homebrew's .git directory is located. For standard installs, the prefix and repository are the same directory.

@wjwwood
Copy link
Member

wjwwood commented Mar 20, 2015

Also the pull request description examples are identical (I think it might have been a copy paste update error).

@jbohren
Copy link
Contributor Author

jbohren commented Mar 20, 2015

Hmm, the find implies searching, but really it's a look up (finding a package does crawl, but in a limited scope).

Yeah, other names are fine. I originally switched it from path because it wasn't very verb-y. I think catkin find and catkin locate would be my top two choices.

Also the pull request description examples are identical (I think it might have been a copy paste update error).

One is bash the other is zsh. Not really necessary to have both there of course.

@wjwwood
Copy link
Member

wjwwood commented Mar 20, 2015

What about this:

catkin locate {--source | -s [pkg_name], --devel | -d, --install | -i, --root | -r}

It combines --source and --pkg, since a package should always be in src somewhere.

@wjwwood
Copy link
Member

wjwwood commented Mar 20, 2015

These would also be good flags to put in the ws verb, so like this:

$ catkin ws --source
/tmp/my_ws/src
$ catkin ws --source rospy
/tmp/my_ws/src/ros_comm/clients/rospy
$ catkin ws --install
/opt/ros/indigo
...

@wjwwood
Copy link
Member

wjwwood commented Mar 20, 2015

Some other things to think about:

  • What is the behavior when a package is not found?
  • What happens if the install or devel space does not exist yet?
  • Should the build space be an option too?
  • Does this respect build profiles?

@jbohren
Copy link
Contributor Author

jbohren commented Mar 20, 2015

These would also be good flags to put in the ws verb

Yeah that could also work, though I think a find or locate verb makes it clearer to use these features.

What is the behavior when a package is not found?

It returns an error code and prints a message to stderr.

What happens if the install or devel space does not exist yet?

Currently it will give you the path to where they will be created. I think it should also return nothing and print an error to stderr.

Should the build space be an option too?

It currently is and yes I think it should be.

Does this respect build profiles?

Yes. It gets the space paths from the context.

@jbohren
Copy link
Contributor Author

jbohren commented Mar 25, 2015

@wjwwood What do you think of this interface:

usage: catkin find [-h] [--workspace WORKSPACE] [--profile PROFILE] [-e] [-r]
                   [-s | -b | -d | -i]
                   [PACKAGE]

Get the paths to various locations in a workspace.

optional arguments:
  -h, --help            show this help message and exit
  --workspace WORKSPACE, -w WORKSPACE
                        The path to the catkin_tools workspace or a directory
                        contained within it (default: ".")
  --profile PROFILE     The name of a config profile to use (default: active
                        profile)

Behavior:
  -e, --existing-only   Only print paths to existing directories.
  -r, --relative        Print relative paths instead of the absolute paths.

Sub-Space Options:
  Get the absolute path to one of the following locations in the given
  workspace with the given profile.

  -s, --src             Get the path to the source space.
  -b, --build           Get the path to the build space.
  -d, --devel           Get the path to the devel space.
  -i, --install         Get the path to the install space.

Package Directories:
  Get the absolute path to package directories in the given workspace and
  sub-space. By default this will output paths in the workspace's source
  space. If the -b (--build) flag is given, it will output the path to the
  package's build directory. If the -d or -i (--devel or --install) flags
  are given, it will output the path to the package's share directory in
  that space.

  PACKAGE               The name of a package to find.

@jbohren jbohren force-pushed the path-verb branch 5 times, most recently from ca84b2e to 2faf8cf Compare April 7, 2015 13:42
@jbohren
Copy link
Contributor Author

jbohren commented Apr 9, 2015

@wjwwood This is the next one in the queue, I believe.

@wjwwood
Copy link
Member

wjwwood commented Apr 10, 2015

Sorry, I've been letting this roll around in the back of my mind. I'm still not sold on using find. I think it should be reserved for replacing catkin_find which does something different than this and is more aptly named in my opinion. To be specific, find implies searching and the possibility for multiple results, which is true for what catkin_find does, but is not true for these actions. For the above options, there exists exactly one answer for each and since it is based on the profiles is just a lookup, not a search. So, I hope I can show that I'm not being capricious, but rather than the find verb really doesn't match these actions well.

I really think these options should be either attached to a different verb name, like locate (which I quite like), and/or added to the proposed ws command since they are ws specific once that is implemented.

Thoughts?

@wjwwood
Copy link
Member

wjwwood commented Apr 10, 2015

Oh, and other than the verb name, the above looks great to me.

@jbohren
Copy link
Contributor Author

jbohren commented Apr 10, 2015

+1 for locate

@wjwwood
Copy link
Member

wjwwood commented Apr 15, 2015

I merged with rename.

@wjwwood wjwwood closed this Apr 15, 2015
@jbohren jbohren changed the title verbs: adding find verb to get workspace paths verbs: adding locate verb to get workspace paths Jun 11, 2015
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