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

use tools.deps.alpha map #60

Merged
merged 5 commits into from
Oct 3, 2019
Merged

use tools.deps.alpha map #60

merged 5 commits into from
Oct 3, 2019

Conversation

mikepjb
Copy link

@mikepjb mikepjb commented Aug 20, 2019

I tested this by creating a new folder with a blank deps.edn then using:

clojure -Sdeps '{:deps {pack/pack.alpha {:git/url "https://github.com/juxt/pack.alpha.git" :sha "dccf2134bcf03726a9465d2b9997c42e5cd91bff"}}}' -m mach.pack.alpha.inject '2769a6224bfb938e777906ea311b3daf7d2220f5'

Which injected pack.alpha into the deps.edn (I then updated this to :local/root to point at my local copy of the project

clj -A:pack mach.pack.alpha.capsule ok.jar

and running:
java -jar ok.jar
which presented me with a Clojure REPL

There was a complain about missing StaticLoggerBinder so a little more work is needed - here is the error output:

test(!) $ clj -A:pack mach.pack.alpha.capsule ok.jar
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
test(!) $ 

@SevereOverfl0w
Copy link
Collaborator

I don't think that message is new :)

@SevereOverfl0w
Copy link
Collaborator

SevereOverfl0w commented Aug 20, 2019

Oh, yes it is. tools.deps.alpha removed their dependency on slf4j-nop. We're now in the awkward position of having to either bring along our own slf4j solution (e.g. -nop) or none at all and leave it to the user.

Technically speaking, the message that's there is OK, if unattractive.

For now, nothing is worse if we add a slf4j-nop dependency.

@mikepjb
Copy link
Author

mikepjb commented Aug 20, 2019

I've included slf4j-nop (basically the reverse of Alex's 00dfa4a113311555ea9738889a295c88213253b3 on tools.deps.alpha)

This removes the warning (still with a successfully build uberjar)

@mikepjb
Copy link
Author

mikepjb commented Aug 20, 2019

I suppose a consequence of including nop is that anyone wanting to use slf4j will need to add nop as an exclusion in their deps.edn file?

@SevereOverfl0w
Copy link
Collaborator

Only if the message bothers them. There's a potential risk that they will end up seeing lots of output otherwise, depending on their config.

This isn't something I have a great solution for unfortunately. There's a related open issue.

Copy link
Collaborator

@SevereOverfl0w SevereOverfl0w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very close, minor change.

io/resource
slurp
edn/read-string))
(tools.deps.reader/read-deps (tools.deps.reader/default-deps)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to use the default-deps here, I don't want :deps from the user's ~/.clojure/deps.edn to be factored into creating uberjars. This should be the install-deps instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this argument to nil as read-deps merges it's arguments into (install-deps) anyway.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been updated again (system-edn is no longer needed as install-deps can be inlined)

@SevereOverfl0w
Copy link
Collaborator

Sorry, dropped the ball on this.

@SevereOverfl0w SevereOverfl0w merged commit 042c614 into juxt:master Oct 3, 2019
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

Successfully merging this pull request may close these issues.

2 participants