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

Reconsider granularity of API #122

Open
cursive-ide opened this issue Jan 20, 2024 · 3 comments
Open

Reconsider granularity of API #122

cursive-ide opened this issue Jan 20, 2024 · 3 comments

Comments

@cursive-ide
Copy link
Contributor

I recently fixed cursive-ide/cursive#2867, which is due to some internal changes in deps.clj (use of the $HOME env var to find the user home). However as part of debugging this I also found another related issue, due to changes in how the cache checksum is calculated. Most of what I need is exposed via the API, but the recent API changes have required code like this:

get-cache-dir (let [ns (find-ns 'borkdude.deps)]
                (or (ns-resolve ns 'get-cache-dir*)
                    (ns-resolve ns 'get-cache-dir)))
cache-result (get-cache-dir {:deps-edn   deps-edn
                             :config-dir config-dir})
cache-dir (if (map? cache-result)
            (:cache-dir cache-result)
            cache-result)
cache-dir-key (if (map? cache-result)
                (:cache-dir-key cache-result))
checksum (deps/get-checksum
           (cond-> {:cli-opts     cli-opts
                    :config-paths config-paths}
             cache-dir-key (assoc :cache-dir-key cache-dir-key)))

i.e. figure out whether to use the old or the new means of getting the cache dir, and then switching on the type of the return to calculate the checksum. I can't see any other way to have this work such that the version of Cursive doesn't explicitly depend on a specific version of deps.clj without doing this sort of workaround.

I don't have a specific proposal here, but it would be nice if we could figure out a way to evolve the API without the need for this sort of thing on the client side. In this case, it looks like the idea was that get-cache-dir would continue to work, but unfortunately without cache-dir-key I can't get consistent checksums, which I need to pass to get-basis-file. If I continued to call get-cache-dir (as opposed to get-cache-dir*), then I wouldn't pass cache-dir-key to checksum, and would get errors like:

java.io.FileNotFoundException: /Users/colin/dev/cursive-bugs/polylith-kaocha/.cpcache/DDAB581C3D901BDA5B33C69859866E56.basis (No such file or directory)

I had thought that the checksum could continue to produce consistent results if cache-dir-key wasn't passed to it, but of course that won't work, and the script would still produce different values if one client (Cursive) passed it and the other (deps.clj itself) did not. Basically, the checksum will have to be updated from time to time if new internal state is added, but that will be difficult to expose through the API in a backwards compatible way. Perhaps a good way to achieve this would be to add the basis file path to the -Sdescribe output? That would not be consistent with the Clojure CLI tools, of course. Or perhaps add a new flag which outputs extra data? The basic idea would be to add a higher-level API (which -Sdescribe really is) which doesn't depend so much on the internal details of how deps.clj works, currently the API is very granular and requires clients to be quite tied to the internals of the script.

@cursive-ide cursive-ide changed the title Expose more details via API Reconsider granularity of API Jan 20, 2024
@borkdude
Copy link
Owner

Sorry for the "breakage" but yeah, the idea was to preserve backward compatibility. This change was necessary since the newest clojure CLI checks if the current working directory is writeable, and if not, it will choose a directory in home that is writeable for the cache key.

I'd be fine adding extra data to -Sdescribe. PR welcome in a way that suits you.
Other proposals also welcome.

@borkdude
Copy link
Owner

@cursive-ide

@puredanger is considering adding -Sbasis to the CLI. Would this solve your problem? I'm not sure if -Sbasis is going to print the file which contains the basis of the contents of that file

@cursive-ide
Copy link
Contributor Author

@borkdude I think that would be fine, since I only want the path to get the basis itself IIRC. I'll check the code today. In retrospect, I can't believe I didn't think of this option!

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

2 participants