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

Excluded or overriden transitive dependencies break topological sort #7

Open
frenchy64 opened this Issue Jun 30, 2015 · 10 comments

Comments

Projects
None yet
2 participants
@frenchy64
Copy link

frenchy64 commented Jun 30, 2015

A project.clj such as this:

:dependencies [[org.clojure/core.memoize "0.5.6"]
               [org.clojure/core.cache "0.6.4"]]

ignores the fact that core.memoize must be processed before core.cache because pomegranate thinks core.cache does not appear in the dependencies of core.memoize.

This means core.memoize ends up with calls to clojure.core.cache, since core.cache can be processed first.

@benedekfazekas

This comment has been minimized.

Copy link
Owner

benedekfazekas commented Jun 30, 2015

yup, i am aware of this problem. my idea to fix it is basically work with an unresolved dependency tree if that makes sense. so if a certain library occures multiple times in the tree say once as a transient dependency and once as a direct dependency i would rewrite and include it twice. hope this makes sense...

@frenchy64

This comment has been minimized.

Copy link
Author

frenchy64 commented Jun 30, 2015

As in, you wouldn't change the current topological sort, but also pass around another tree that includes the "actual" dependencies?

Do you get this unresolved tree from Aether by just deleting the :exclusions form?

And does this mean you would mangle core.cache requires twice? Once for core.cache, then again when you discover core.memoize also depends on it?

@benedekfazekas

This comment has been minimized.

Copy link
Owner

benedekfazekas commented Jun 30, 2015

i want to cheat in the sense that i would loop through the direct depedencies and pretend that they are the only dependency. in other words work with the one tree per direct dependency. (i have not proven that this would work yet)

unresolved tree from aether

i guess not because it would still 'solve' clashing versions etc

mangle core.cache twice

this would only make sense if i separated the processing of the above mentioned trees. can get tricky when i replace the references in the original project code. this might result in a system which does not allow you to use transient dependencies directly in your code -- which may be a good thing tbh

@frenchy64

This comment has been minimized.

Copy link
Author

frenchy64 commented Jun 30, 2015

Your cheat sounds like the easiest solution.

@benedekfazekas

This comment has been minimized.

Copy link
Owner

benedekfazekas commented Jun 30, 2015

let me know if you have some ideas around this tho

@frenchy64

This comment has been minimized.

Copy link
Author

frenchy64 commented Jun 30, 2015

It seems like if we fiddle with the topological map from Aether this fix would be a one liner. My idea was to grab the resolved topological map, then walk over it, adding dependencies that have been overriden and reordering as needed. Then just plug this new topological map into the original functions.

@benedekfazekas

This comment has been minimized.

Copy link
Owner

benedekfazekas commented Jun 30, 2015

i guess the tricky thing with 'cheat' would be (1) seperating the trees (quite a bit hassling with directories etc) and (2) updating the references at the right places only

@benedekfazekas

This comment has been minimized.

Copy link
Owner

benedekfazekas commented Jul 12, 2015

the possible problems with my approach:

  • even bigger jar as a result
  • even slower loading time as the same dependency will be loaded several times if it occurs at multiple places

benedekfazekas added a commit that referenced this issue Jul 12, 2015

[#7] sort direct depencies acc to their frequency
- resolve direct dependencies one by one
- walk the resulting tree and count all occurrances of dependencies
- order the first level dependencies according to their number of
  occurrence, if they occure multiple times they will be resolved later
@benedekfazekas

This comment has been minimized.

Copy link
Owner

benedekfazekas commented Jul 12, 2015

I came up with a hybrid solution where as a best effort I order the primary dependencies according to how many times they occur in the dependency tree: more times they occur the later they get resolved. I guess this approach fails if primary dependency A occurs more times than B but A actually depends on B. However, this solution solves the simpler cases (the one which pending on mranderson for long time in refactor-nrepl for example).

Let me know what you think. 0.4.4-SNAPSHOT is on clojars btw if you want to play with it

benedekfazekas added a commit that referenced this issue Jul 19, 2015

Revert "[Fix #7] sort dependencies acc their inter dependencies"
This reverts commit 16501a6.

fall back to sort dependencies acc to their frequencies
@benedekfazekas

This comment has been minimized.

Copy link
Owner

benedekfazekas commented Jul 19, 2015

had to revert my failing try to sort deps. i guess options still are

  • implement proper topological sort
  • original idea of processing every first level dependency independently (won't help with performance)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment