tools.deps

Problem with local deps with local deps

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

Local deps can refer to other local deps with either absolute or relative paths. Absolute paths are unambiguous but not portable across members of a team or when pulling down a monorepo git dep with internal local deps.

Repro:

a/
  deps.edn
b/
  deps.edn
  libs/
    c/deps.edn
 
    
# in a
b {:local/root "../b"}
# in b
c {:local/root "./libs/c"}

clj -Spath
src:
local-deps-bug/a/../b/src
local-deps-bug/a/libs/c/src

# but should be

local-deps-bug/b/libs/c/src

# or

local-deps-bug/../b/libs/c/src

See sample repo - https://github.com/niquola/local-deps-bug

Cause: The main problem here is that when using relative paths in the deps.edn, they have to be relative to something. The current assumption made is that they are relative to the current directory (which is wrong once you traverse to a dep).

Places where relative paths can appear:

  • :local/root - used in a deps.edn local dep to refer to the directory containing the manifest file of the dep. Should be relative to the current deps.edn file.
  • :deps/root - used in a deps.edn git dep to refer to a subdirectory relative to the git working tree root
  • :paths - relative to the location of the manifest file

Additionally, some environments (like Cursive) do not have a meaningful "current directory" so relying on relative paths at all is prone to error.

Proposed: While traversing and resolving dependencies, keep track of a directory context. Allow the starting context to be set explicitly on invocation (scripts can default to current directory). It might be convenient for this context to have stack-like properties (as you traverse to a dep you want to push a new directory on the stack etc), however, since the dependency traversal is iterative, not recursive, this may actually not be necessary. There are several possible places to put this context:

  • The config map. Currently the config is basically static. Putting it in the config means we will be modifying it and making it context-dependent, which could increase chance of mistakes.
  • Explicit parameter. Could be passed around in addition to the config. Probably affects dozens of call sites, so messy breaking change.
  • Dynamic var. While I am rarely a fan of implicit state, this may actually be a good place for it. Works well for per-thread (might be useful if we parallelize), has stack properties if needed. Minimizes param changes. With some standard helper functions could minimize the code that uses it.
  • Other ambient state (var/atom, threadlocal, etc) - same implicit downsides as dynamic
    vars and not as good in other ways.

Activity

Hide
Matthias Margush added a comment -

Here's another sample project that illustrates the issue.

This is a blocker for one of our projects, so I've put together a temporary fix.

Show
Matthias Margush added a comment - Here's another sample project that illustrates the issue. This is a blocker for one of our projects, so I've put together a temporary fix.
Hide
Matthias Margush added a comment -

Hi Alex. I've added a test for this and attached a patch for discussion. Thanks!

Show
Matthias Margush added a comment - Hi Alex. I've added a test for this and attached a patch for discussion. Thanks!
Hide
Alex Miller added a comment -

I don't like shoving stuff into the config in the patch, also doesn't go far enough in analyzing all the ramifications of relative paths computations throughout. I've updated the description to cover this better.

Show
Alex Miller added a comment - I don't like shoving stuff into the config in the patch, also doesn't go far enough in analyzing all the ramifications of relative paths computations throughout. I've updated the description to cover this better.
Hide
Alex Miller added a comment -

Made commit 746be01d6b19fe354420c143a5b1ad92e0a78f3f for this. Not yet released. Still want to look at issue of specifying initial dir via api.

Show
Alex Miller added a comment - Made commit 746be01d6b19fe354420c143a5b1ad92e0a78f3f for this. Not yet released. Still want to look at issue of specifying initial dir via api.
Hide
Alex Miller added a comment -

Committed and released in tools.deps.alpha 0.6.496

Show
Alex Miller added a comment - Committed and released in tools.deps.alpha 0.6.496

People

Vote (15)
Watch (8)

Dates

  • Created:
    Updated:
    Resolved: