weavejester / environ

Library for managing environment variables in Clojure
922 stars 71 forks source link

Lein 2.8.2 warns about deprecated hook lein-environ.plugin/hooks #88

Open a613 opened 5 years ago

a613 commented 5 years ago

After upgrading from lein 2.8.1 to 2.8.2:

$ lein --version
Warning: implicit hook found: lein-environ.plugin/hooks
Hooks are deprecated and will be removed in a future version.
Leiningen 2.8.2 on Java 1.8.0_151 Java HotSpot(TM) 64-Bit Server VM
extremus commented 5 years ago

I had the same warning with Leiningen 2.8.3 on Java 10.0.2. I've changed the :plugin section to [ lein-environ "1.1.0" :hooks false ] and it's now ok in my project.

a613 commented 5 years ago

@extremus thanks, that works fine in my case too.

terjedahl commented 5 years ago

Yes, adding :hooks false does get rid of the warning, but doesn't it also then disable writing :env values in project.clj and profiles.clj from being written to .lein-env; thereby disabling the most useful feature of the plugin? Can the plugin be re-written in such a way that it no longer relies on hooks?

darwin commented 5 years ago

@terjedahl is correct. Disabling hooks will stop writing .lein-env. It seems to work just because the old file stays in place until you start from scratch (e.g. clone the repo somewhere else).

The whole idea of writing to a file which is then consumed by the library is quite brittle. For example I had a lot of headaches when using this with lein-shell plugin. Imagine running a shell script from leiningen which then happens to call some other lein command (you end up with nested lein processes). This way only inner-most process wins writing the file and others get wrong environments depending on who was last writer to the file when they happen to read it.

You have to understand implementation details of this plugin to see what went wrong. I would recommend opening a PR in leiningen itself and implement independent support for ENV variables there. Dynamic .lein-env is not a good idea.

weavejester commented 5 years ago

If anyone can come up with a better implementation for passing the environment data from the Leiningen project to the application process, I'd welcome a PR.

terjedahl commented 5 years ago

One workaround could be to replace the hook-mechanism with a middleware mechanism.
I understand that this would be conceptually wrong, as middleware shouldn't have side-effects. We wouldn't be actually be altering the project-structure either. And also, as implicit middleware is going away, the user would still have to declare it explicitly as :middleware.

Another solution might be to simply have it hook in via the :prep-tasks mechanism. This could possibly even alleviate @darwin 's issue as it could be added selectively in profiles.

wesleyhall commented 5 years ago

This is admittedly an idle thought, but figwheel seems to be able to pull information out of the project.clj without the need for the hooks mechanism. I wonder if a similar approach can be adopted here. Unless anybody tells me i'm crazy, i'll keep the warning in my project as a reminder to take a look as soon as I can.

terjedahl commented 5 years ago

I was thinking along similar lines as you @wesleyhall: As environ.core/env is a result of calling environ.core/read-env, which in turn amongst other reads from file '.lein-env', why not skip the intermediate step of generating the '.lein-env' file and instead read directly from the 'project.clj'. The challenge might be in detecting/applying profiles, though ... :-/

wesleyhall commented 5 years ago

@terjedahl

I did take a very quick look into this last night and Bruce @ Figwheel actually has this little project:

https://github.com/bhauman/simple-lein-profile-merge

He doesn't seem particularly enthusiastic about it being ready for wider adoption, but it is out there.

kengruven commented 5 years ago

I just spent a couple days learning way more than I ever wanted to about lein and environ, because I thought I was helping myself by silencing the "Warning: implicit hook found" message. Oops.

In the short term, how do you feel about a note in the README that warns idiots like me not to use :hooks false in their project.clj, even if they see red warnings in their logs?

radhikalism commented 4 years ago

There seems to be two issues under discussion here. One is about avoiding the Leiningen implicit hooks warning (but keeping the brittle .lein-env file strategy). The other is about finding a different way to communicate project maps to the project runtime.

On the first question of implicit hooks alone:

rome-user commented 1 year ago

is there any reason lein-environ cannot use an explicit hook instead of an implicit hook, at least, to avoid this warning?

Hooks in general are scheduled to be removed in a future release of Leiningen. Anything that keeps using hooks is NG. This leaves us with the :prep-tasks question. I assume the problem with prep task is the inability of plugins to add their own prep task. The user may have to configure it manually.