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

"RELEASE" does not need to be updated #7

Closed
seancorfield opened this issue Nov 13, 2018 · 16 comments
Closed

"RELEASE" does not need to be updated #7

seancorfield opened this issue Nov 13, 2018 · 16 comments

Comments

@seancorfield
Copy link
Contributor

When using the --update option, it shouldn't rewrite "RELEASE" to a specific version.

@Olical
Copy link
Owner

Olical commented Nov 14, 2018

Ah, I guess this will just be a special case. I can take a look at this soon unless you want to beat me to it @plexus? I don't mind picking it up at all, it's here if you want it though.

@plexus
Copy link
Contributor

plexus commented Nov 14, 2018

I didn't know "RELEASE" was a thing. Is this a tools.deps or a Maven thing?

@Olical
Copy link
Owner

Olical commented Nov 14, 2018

Neither did I 😅 this shines some light on it https://stackoverflow.com/questions/45567787/maven-release-and-latest-version-numbers

Apparently it's deprecated in Maven 3? I think the features were dropped 6 years ago to help make builds a little more reproducible...

Maybe LATEST and RELEASE need to be excluded from automatic updating for historical reasons though, even if they're not supposed to be used anymore. I personally really dislike anything that can potentially fetch a different version on every run. Specific versions and lock files, please 🎉

@plexus
Copy link
Contributor

plexus commented Nov 14, 2018

Interesting! And certainly occasionally useful :)

Should be easy enough to exclude them. I'll prep a patch.

@Olical
Copy link
Owner

Olical commented Nov 14, 2018 via email

@Olical
Copy link
Owner

Olical commented Nov 14, 2018

@seancorfield How's this work for you? Seems fine to me code and functionality wise. Nice to see 1.8 come back too, I didn't think it was a massive issue but it's a nice touch. @plexus works fast! 😱

clj -Sdeps '{:deps {olical/depot {:git/url "https://github.com/plexus/depot.git" :sha "bb22f11bab5a9dda0c6013268e20eebb6cc70852"}}}' -m depot.outdated.main --update

@seancorfield
Copy link
Contributor Author

That seems to work just fine!

As for the use case: I use "RELEASE" with all my dev tooling so that it stays fresh without needing any manual intervention. If anything breaks, then I debug and fix a version, but that's pretty rare.

I noticed that --update doesn't modify :override-deps (I suspect it doesn't even look at them), which may be good or bad, depending on your point of view. In my use case, https://github.com/seancorfield/dot-clojure/blob/master/deps.edn#L4-L14 it is good that it doesn't mess with them, but that is also how we set all our dependency versions in our work setup so we would still rely on the outdated report there instead of letting Depot update it (and for some libraries it is very deliberate that we're on an older version).

@Olical Olical closed this as completed in #9 Nov 15, 2018
@plexus
Copy link
Contributor

plexus commented Nov 15, 2018

I hadn't thought of :override-deps, I think we should by default check and update those as well. If you want to stick to an outdated/fixed version there's ^:depot/ignore, that's what it's intended for.

@Olical
Copy link
Owner

Olical commented Nov 15, 2018

Ah, I haven't deployed yet. Maybe this would be good to add first, I was going to comment on this after deploying since I thought it wasn't really related. I then sipped my morning coffee and realised it was.

So the normal "just show me what's outdated" does check :override-deps, it makes sense that --update should be consistent with that to me.

@Olical Olical reopened this Nov 15, 2018
@Olical
Copy link
Owner

Olical commented Nov 15, 2018

@seancorfield how's that look for you now? @plexus has nailed another PR 😄

clj -Sdeps '{:deps {olical/depot {:git/url "https://github.com/Olical/depot.git" :sha "3af4ceaaa47fe0a8bf057e2c8963ff1697f83b25"}}}' -m depot.outdated.main --update

@seancorfield
Copy link
Contributor Author

Well, I specifically don't need/want that functionality but I'm fine with it being in there :)

I will observe that with the outdated report, you need to specify one or more aliases to have things checked, but --update just checks (and updates) everything, regardless of aliases, so that might be an inconsistency you want to address one way or another.

In our monorepo, we have multiple subprojects that each have a deps.edn and we have a special subproject called versions that has all our aliases and override deps etc, so we could run --update in versions and it would update the overrides, but if we run it in a regular subproject, we need to provide CLJ_CONFIG=../versions and -A:defaults to pick up those -- because we specify dep coords as {} in each subproject -- and I'm not sure what --update makes of {}? It probably doesn't think it's a Maven/Clojars coordinate at all?

Anyways, thanks for fixing the "RELEASE" (and "LATEST") coordinate handling!

@Olical
Copy link
Owner

Olical commented Nov 19, 2018

Released in 1.5.0. Thank you for the ideas, discussion and work. 👏

@plexus
Copy link
Contributor

plexus commented Nov 19, 2018

--update just checks (and updates) everything, regardless of aliases, so that might be an inconsistency you want to address one way or another.

I'll admit I implemented it this way as this is usually what I want, that said I'm happy to help improve this assuming we can agree on how it should work. The regular version checks uses tools.deps to resolve all versions based on system-wide deps.edn, user-specific deps.edn, and the project local one, as well as any aliases therein. For --update we need to trace back where specific versions came from, making it a bit more tricky. E.g. what if a library appears in multiple places? (files and/or aliases), do we update all of them?

I'm not sure what --update makes of {}? It probably doesn't think it's a Maven/Clojars coordinate at all?

That's right, currently it looks for a :mvn/version or :sha, if neither is there it's left alone.

@seancorfield
Copy link
Contributor Author

From our point of view -- with a monorepo and multiple deps.edn files and lots of aliases to manage tooling etc -- it would be really nice if the regular out-of-date check worked like the --update option, reading the deps.edn file in the current directory and checking all :mvn/version and :sha versions, regardless of aliases.

We have a script that runs every day and drops into each subproject in our repo and runs the outdated check, but because tools.deps throws an exception on unknown aliases, we can only use the set of aliases that are in our base deps.edn or that are common to every project. That means we can't check versions for certain tooling where an alias exists only in one subproject, unless we special case the aliases in the script, or we load and parse the EDN file to get the list of available aliases -- neither of which is an appealing option.

We're an edge case, I'm sure, but just wanted to provide more feedback on how we use the tool. It's very useful as-is, but it could be more useful if it checked versions the same way the --update option works. :)

@plexus
Copy link
Contributor

plexus commented Nov 20, 2018

Currently Depot uses tools.deps to read the dependency map, letting it resolve any system/user settings as well as aliases. It then works on this resolved dependency map.

If I understand you correctly you'd like to see a mode where instead it reads in deps.edn directly, considering each dependency in :deps or :aliases in isolation.

I can certainly see the usefulness of such a thing. If we add this, should it replace the current behavior? Or do we support both with a command line flag, and if so which one becomes the default? @Olical what do you think?

@seancorfield
Copy link
Contributor Author

If I understand you correctly you'd like to see a mode where instead it reads in deps.edn directly, considering each dependency in :deps or :aliases in isolation.

That would be more useful to us, yes. We can and will continue to use Depot as it stands but we'd prefer at least the option of a mode where all dependencies were examined, as --update does.

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

No branches or pull requests

3 participants