search

habitat-sh / habitat

Modern applications with built-in automation
https://www.habitat.sh
Apache License 2.0
2.59k stars 315 forks source link

Documentation for pkgPathFor is incorrect #6173

Open stevendanna opened 5 years ago

stevendanna commented 5 years ago

The documentation for pkgPathFor (https://www.habitat.sh/docs/reference/#pkgpathfor-helper) says:

Returns the absolute filepath to the package directory of the package best resolved from the given package identifier. The named package must exist in the pkg_deps of the plan from which the template resides. The helper will return a nil string if the named package is not listed in the pkg_deps. As result you will always get what you expect and the template won't leak to other packages on the system.

However, pkgPathFor actually pulls from the TDEPS of a package meaning that it will return valid results for packages which aren't listed in pkg_deps but are transitive dependencies of packages listed in pkg_deps.

While this might have been a bug originally, it is actually very useful for "wrapper" packages in which you may need to replicate hooks from the underlying packages but don't want to introduce constant version conficts by depending directly on script dependencies.

If we are OK with the current behavior (it would be hard to change now as it would mean some set of older package would no longer run), then let's update the documentation.

yzl commented 5 years ago

For whoever picks this up, it might be worth mentioning that pkg_path_for (in plans) does look to pkgDeps, but pkgPathFor (in hooks and config) looks to TDEPS. I don't know if that is the intended behavior, but it is the behavior I observed.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We value your input and contribution. Please leave a comment if this issue still affects you.

jsirex commented 4 years ago

It is very comfortable in a wrapper package that you can relay on TDEPS - it just handy. However, for reliability it better to explicitly use DEPS, not TDEPS. TDEPS may change after upgrade and package you are relaying on might go away.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We value your input and contribution. Please leave a comment if this issue still affects you.