v2 does not check user-level deps.edn

seancorfield commented 5 years ago

I'm pulling this out of #25 as an unresolved issue since that got merged and I don't want this to be lost. The repro case is in this comment:

https://github.com/Olical/depot/pull/25#issuecomment-529723033

Olical commented 5 years ago

Oh damn, sorry about that! I didn't realise that got closed.

On Wed, 18 Sep 2019, 21:00 Sean Corfield, notifications@github.com wrote:

I'm pulling this out of #25 https://github.com/Olical/depot/pull/25 as an unresolved issue since that got merged and I don't want this to be lost. The repro case is in this comment:

25 (comment)

https://github.com/Olical/depot/pull/25#issuecomment-529723033

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Olical/depot/issues/32?email_source=notifications&email_token=AACM6XMX2G6LXD2AWP744YLQKKCERA5CNFSM4IYDDZ42YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HMHTPTQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AACM6XK2ENI32IUTWKXQFBTQKKCERANCNFSM4IYDDZ4Q .

lverns commented 5 years ago

I'm running version 1.10.1.469 of the clojure command line tools on bash 5.0.7.

I've set up a project as described in your comment, but added the following alias to repo/versions/deps.edn:

:depot
{:extra-deps {olical/depot {:mvn/version "1.8.4"}}
 :main-opts ["-m" "depot.outdated.main"]}

Then from repo/subproject, I can run

$ CLJ_CONFIG=../versions clj -A:defaults:depot
All up to date!

I can also do the following:

$ CLJ_CONFIG=../versions clj -A:defaults:depot -a defaults

|          Dependency | Current | Latest |
|---------------------+---------+--------|
| org.clojure/clojure |   1.9.0 | 1.10.1 |

Oh. Oh my. This is definitely not going to work in the new world.

To confirm, I'll change the :depot alias to point to the tip of master:

:depot
{:extra-deps {olical/depot {:git/url "git@github.com:Olical/depot.git"
                :sha "cc9e732f32a1cfb74c5427f5d36187b0dfa447e6"}}
 :main-opts ["-m" "depot.outdated.main"]}}

And rerun:

$ CLJ_CONFIG=../versions clj -A:defaults:depot -a defaults
Checking for old versions in: deps.edn
  All up to date!

Okay, that is definitely a breaking change.

Two questions for @seancorfield: (1) Is this bug the you're talking about or have I run into something subtly different? (2) If it is what you're talking about, can you try the following?

$ CLJ_CONFIG=../versions clj -A:defaults:depot -a defaults deps.edn ../versions/deps.edn
Checking for old versions in: deps.edn
  All up to date!
Checking for old versions in: ../versions/deps.edn
  org.clojure/clojure {:mvn/version "1.9.0"} -> {:mvn/version "1.10.1"}

seancorfield commented 5 years ago

Yes, because the new version doesn't use the environment that it runs in whereas 1.8.4 does. 1.8.4 reflects the actual dependencies you would be running with -- so you can version check particular "paths" through your dev/test/build environment.

lverns commented 5 years ago

@seancorfield Could you change the bug title to v2 does not check user-level deps.edn? "Respect" is a little vague.

In v1.8.4 the handling of this scenario was different when the --update flag was present than when it was absent, and in my implementation of v2, we ended up with the handling from the --update code path.

I'll propose a solution (or solutions) after I've thought about it some more.

seancorfield commented 5 years ago

Title changed. It's not really about checking the user-level deps.edn tho', it's about using the same environment that clj/clojure would for your code, using the same merge process that t.d.a. does.

lverns commented 5 years ago

This breakage comes down to the fact there were two different behaviors in v1.8.4.

Without the --update flag, tools.deps.alpha was used to build the list of libraries to check and was thus able to consider dependencies in the local deps.edn, the user level deps.edn, and the install level deps.edn. This was pretty powerful. For example, when using :override-deps in an alias, the version in the main :deps doesn't matter and depot could limit its report to the version that actually mattered in a given scenario.
With the --update flag, only the passed in files (defaulting to the local deps.edn) were taken into consideration.

v2 attempts to unify the drift between the writing and non-writing branches. Here are the options, I think:

A Bring the understanding of deps.edn merging to the writing branch. This seems hard, and I suspect it would require re-implementing logic that exists in tools.deps. Also, it would allow depot to write to user level or install level deps.edn files which runs the risk of being un-noticed -- since those files are less likely to be under version control, or failing -- since the install level deps.edn is usually read-only.
B Make the non-writing branch check whatever files the user specifies (e.g ./deps.edn, ../other-project/deps.edn etc), instead of using tools.deps.alpa to build the list of libraries. (This is what is currently on the tip of master.)
C Make the branches as similar as possible, but live with (and document) the fact that the set of libs checked can always be different in the two modes.

I lean strongly towards B because I think it provides the most consistent experience and provides workarounds for bugs like #23. I kinda understand the advantages of A, but they aren't something I currently need, and thus I have little motivation to fix this bug.

I am, however, motivated to not make anyone's life worse and it seems that my changes have made at least @seancorfield's experience with this tool worse, so... hmmmmm...

seancorfield commented 5 years ago

If it helps you decide, I've never wanted the --update path (I have always considered it a bad idea) so at this point, I'm perfectly happy to stay on 1.8.4 indefinitely and if I find bugs, I'll simply fork it from that tag and maintain my own version for use at work (and I would probably strip the update logic completely to make it simpler to maintain).

The B option really only works if you combine the contents of the supplied deps.edn files using the same merge strategy that t.d.a. uses -- and to have it respect aliases properly -- which would essentially make it a variant of A anyway (which I still think is the correct behavior for this sort of tool).

sundbp commented 4 years ago

Upgrading to 2.0.1 I also hit the same error as referred to in #25 Execution error (AssertionError) at depot.zip/zassoc (zip.clj:66).. I also have a monorepo style setup.

seancorfield commented 4 years ago

Seeing a recent comment on this still-open issue spurred me to come back with an update:

I stopped using Depot altogether and built a dependency-checking shell script, built on top of clojure -Stree (run against your project with aliases, and then against those same deps in a new project with the version set to "RELEASE", and compares the output). Cheap, cheerful, works great.

So at this point, I don't "care" what Depot decides to do although I will reiterate my comment that trying to do things by reading deps.edn instead of using tools.deps.alpha and supporting aliases etc properly is just a broken approach in the deps.edn world.

Olical commented 3 years ago

Going to close this since I can't see myself addressing it without a rewrite, I'm essentially in light touch maintenance mode with this project for now. It got smarter in a way I wasn't comfortable with, too smart for me 😅 bearing all the ideas and discussion in mind though, it was very helpful!

Olical / depot

v2 does not check user-level deps.edn #32

25 (comment)