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 :main-opts in -Sdeps map during jack in with clojure cli #2941

Closed
dpsutton opened this issue Dec 8, 2020 · 15 comments
Closed

use :main-opts in -Sdeps map during jack in with clojure cli #2941

dpsutton opened this issue Dec 8, 2020 · 15 comments
Labels

Comments

@dpsutton
Copy link
Contributor

dpsutton commented Dec 8, 2020

if you jack in with other profiles that include main opts, the cider commandline main opts are ignored. One way around this is to use the merging resolution for multiple profiles: last main-opts "wins".

ie instead of

/usr/local/bin/clojure -Sdeps '{:deps {nrepl {:mvn/version "0.8.3"} cider/cider-nrepl {:mvn/version "0.25.5"}}}' -A:dev:test -m nrepl.cmdline --middleware '["cider.nrepl/cider-middleware"]'

we can use

clojure -Sdeps '{:deps ... :aliases {:cider/jack-in {:main-opts ["-m" ...]}}}' -M:dev:test:cider/jack-in --middleware ...

this ensures that if any main opts are in dev or test, the cider/jack-in alias will win.

The motivating example is the following alias:

:test {:extra-paths ["test"]
         :extra-deps {lambdaisland/kaocha           {:mvn/version "1.0.690"}
                      lambdaisland/kaocha-cloverage {:mvn/version "1.0.63"}
                      lambdaisland/kaocha-junit-xml {:mvn/version "0.0.76"}
                      ring/ring-mock                {:mvn/version "0.3.2"}
                      mockery                       {:mvn/version "0.1.4"}
                      http-kit.fake/http-kit.fake    {:mvn/version "0.2.1"}}
         :main-opts  ["-m" "kaocha.runner"]}

where a repl is desired with the test library but without running the test lib's main or requiring splitting the main from the necessary dependencies.

@dpsutton
Copy link
Contributor Author

dpsutton commented Dec 8, 2020

repro:

deps.edn

{:paths ["."]
 :aliases
 {:test {:extra-paths ["test"]
         :extra-deps {lambdaisland/kaocha           {:mvn/version "1.0.690"}
                      lambdaisland/kaocha-cloverage {:mvn/version "1.0.63"}
                      lambdaisland/kaocha-junit-xml {:mvn/version "0.0.76"}
                      ring/ring-mock                {:mvn/version "0.3.2"}
                      mockery                       {:mvn/version "0.1.4"}
                      http-kit.fake/http-kit.fake    {:mvn/version "0.2.1"}}
         :main-opts  ["-m" "kaocha.runner"]}}}

tests.edn

{:kaocha/tests    [{:kaocha.testable/type :kaocha.type/clojure.test
                    :kaocha.testable/id   :unit
                    :kaocha/ns-patterns   [".*"]
                    :kaocha/source-paths  ["src"]
                    :kaocha/test-paths    ["test"]}]
 :kaocha/reporter [kaocha.report/dots]}

test/file.clj

(ns file
  (:require [clojure.test :refer [deftest is]]))

(deftest f (is (= 1 1)))

running:

clj -A:test
[(.)]
1 tests, 1 assertions, 0 failures.

when using cider-jack-in:

[nREPL] Starting server via /usr/local/bin/clojure -Sdeps '{:deps {nrepl {:mvn/version "0.8.2"} cider/cider-nrepl {:mvn/version "0.25.4"}}}' -A:test -m nrepl.cmdline --middleware '["cider.nrepl/cider-middleware"]'
error in process sentinel: cond: Could not start nREPL server: DEPRECATED: Libs must be qualified, change nrepl => nrepl/nrepl 
DEPRECATED: Libs must be qualified, change mockery => mockery/mockery (deps.edn)
WARNING: Use of :main-opts with -A is deprecated. Use -M instead.
WARNING: When invoking clojure.main, use -M
Unknown option: "-m"
Unknown option: "--middleware"
USAGE:

bin/kaocha [OPTIONS]... [TEST-SUITE]...

  -c, --config-file FILE  tests.edn  Config file to read.
      --print-config                 Print out the fully merged and normalized config, then exit.
      --print-test-plan              Load tests, build up a test plan, then print out the test plan and exit.
      --print-result                 Print the test result map as returned by the Kaocha API.
      --fail-fast                    Stop testing after the first failure.
      --[no-]color                   Enable/disable ANSI color codes in output. Defaults to true.
      --[no-]watch                   Watch filesystem for changes and re-run tests.
      --reporter SYMBOL              Change the test reporter, can be specified multiple times.
      --plugin KEYWORD               Load the given plugin.
      --profile KEYWORD              Configuration profile. Defaults to :default or :ci.
      --version                      Print version information and quit.
      --help                         Display this help message.
  -H, --test-help                    Display this help message.

Options may be repeated multiple times for a logical OR effect.

error in process sentinel: Could not start nREPL server: DEPRECATED: Libs must be qualified, change nrepl => nrepl/nrepl 
DEPRECATED: Libs must be qualified, change mockery => mockery/mockery (deps.edn)
WARNING: Use of :main-opts with -A is deprecated. Use -M instead.
WARNING: When invoking clojure.main, use -M
Unknown option: "-m"
Unknown option: "--middleware"
USAGE:

bin/kaocha [OPTIONS]... [TEST-SUITE]...

  -c, --config-file FILE  tests.edn  Config file to read.
      --print-config                 Print out the fully merged and normalized config, then exit.
      --print-test-plan              Load tests, build up a test plan, then print out the test plan and exit.
      --print-result                 Print the test result map as returned by the Kaocha API.
      --fail-fast                    Stop testing after the first failure.
      --[no-]color                   Enable/disable ANSI color codes in output. Defaults to true.
      --[no-]watch                   Watch filesystem for changes and re-run tests.
      --reporter SYMBOL              Change the test reporter, can be specified multiple times.
      --plugin KEYWORD               Load the given plugin.
      --profile KEYWORD              Configuration profile. Defaults to :default or :ci.
      --version                      Print version information and quit.
      --help                         Display this help message.
  -H, --test-help                    Display this help message.

Options may be repeated multiple times for a logical OR effect.

Quit

here the -m nrep.cmdline etc are passed as arguments to the kaocha main

@bbatsov bbatsov added the bug label Dec 9, 2020
@bbatsov
Copy link
Member

bbatsov commented Dec 9, 2020

I'm not using the clojure cli, so I'll defer to you on this one. PR welcome!

@practicalli-johnny
Copy link
Contributor

practicalli-johnny commented Dec 9, 2020

EDIT: use cider-clojure-cli-aliases to include project and user aliases in the cider-jack-in command. Do not use cider-clojure-global-options to include aliases as this causes the cider-jack-in command to fail.

Using the :env/test alias with the :middleware/cider-clj from the practicalli/clojure-deps-edn configuration in the last position in the alias chain works with cider-jack-in-* without any issues

/usr/local/bin/clojure -Sdeps '{:deps {nrepl {:mvn/version "0.8.3"} cider/cider-nrepl {:mvn/version "0.25.4"}}}' -M:env/test:middleware/cider-clj -m nrepl.cmdline --middleware '["cider.nrepl/cider-middleware"]'

I am unclear why adding to the complexity of the -Sdeps auto-injection would be valuable as I assume it also requires code to parse all the aliases supplied to see if they have an alias. It seems strange just to force aliases into -Sdeps and would be a good reason to just switch to cider-connect and abandon cider-jack-in for all but the most simple of cases.

As an aside, as kaocha is not actually used in this example as the repo is only using cider-test.
The defacto approach is to create a :runner or :test/runner alias that would run kaocha. Having two aliases ensures you can load the test directory with the :test alias which is needed for cider-test. And only load in all the kaocha dependencies (there are quite a few) when actually running kaocha.
I've put a lot of design into the practicalli/clojure-deps-edn configuration to provide crafted aliases for many situations. Hopefully you find that useful in your deliberations.

@bbatsov
Copy link
Member

bbatsov commented Dec 9, 2020

I am unclear why adding to the complexity of the -Sdeps auto-injection would be valuable as I assume it also requires code to parse all the aliases supplied to see if they have an alias.

The existing functionality is just a direct port of the original injection which was for Leiningen and where such problems simply didn't exist. Whatever got passed over the command-line simply overwrite the other specified deps, which is ideal as it allows CIDER to make sure people are using the right version of nREPL, cider-nrepl, etc. The biggest caveat of the cider-connect approach is that you have to think about the version of deps, although if you update often you should be fine.

@dpsutton
Copy link
Contributor Author

dpsutton commented Dec 9, 2020

As an aside, as kaocha is not actually used in this example as the repo is only using cider-test.
The defacto approach is to create a :runner or :test/runner alias that would run kaocha. Having two aliases ensures you can load the test directory with the :test alias which is needed for cider-test. And only load in all the kaocha dependencies (there are quite a few) when actually running kaocha.
I've put a lot of design into the practicalli/clojure-deps-edn configuration to provide crafted aliases for many situations. Hopefully you find that useful in your deliberations.

kaocha is explicitly used in this example. the profile includes extra deps and a main, as exemplified by the results of clj -A:test and the resultant output. If someone wished to have these testing libraries on the classpath so they could run tests from the repl as indicated in their documentation a repl fails to start up.

You mentioned this example uses "cider test" but I don't know what that is. An alternative approach to make a runner alias is not actually an alternative here as that is exactly what the -A:test alias is doing. However, if someone wished to take advantage of the test features provided by kaocha but run them from the repl, that is not currently possible as demonstrated by this real world example.

@dpsutton
Copy link
Contributor Author

dpsutton commented Dec 9, 2020

I am unclear why adding to the complexity of the -Sdeps auto-injection would be valuable as I assume it also requires code to parse all the aliases supplied to see if they have an alias.

I don't see why we need code to parse aliases. We always use an inline -Sdeps to include our dependencies. The proposal is to include the main opts there rather than inline at the end of the string so that we can ensure that -m nrepl.cmdline --middleware '%s' is always the main invoked by the clojure process. If we put this in the startup command we can ensure that this is always the main entrypoint regardless if any other aliases have main opts or not.

It seems strange just to force aliases into -Sdeps and would be a good reason to just switch to cider-connect and abandon cider-jack-in for all but the most simple of cases.

I'm not following this point. This issue is only under cider-jack-in and makes that function more robust.

@kirillsalykin
Copy link

kirillsalykin commented Dec 10, 2020

I actually not sure I am following the comments from @jr0cket

The problem is that any used c.t.d. alias can break the cider-jack-in because alias can specify the -m.

It has nothing to do with kaocha particularly, it is just the example.

The approach as (I see it) generate the cider alias with deps and -m for cider and just add it to the cider-clojure-cli-global-options. so instead of -Sdeps '{:deps {nrepl {:mvn/version "0.8.2"}... cider will wrap it with alias and append the cider alias to the cider-clojure-cli-global-options.

This way it follows the latest c.t.d. approach and ensures the main is -m nrepl.cmdline

Maybe I am missing something - but I don't see any complexity with it.

Offtopic:
having second runner alias just for the invoking -m seems redundant to me, if there is a possibility just to override -m with a standard approach.

@kirillsalykin
Copy link

I may try to make the PR (dont have much elisp experience tho)

@practicalli-johnny
Copy link
Contributor

Seems I am unable to make my points clear in this and previous issues and PRs. It seems we don't have a shared view on how to proceed with Clojure CLI support and I haven't been able to communicate my approach or concerns so you understand them, so will retire from this topic and switch to using cider-connect which seems to be a better workflow in the long run and I am not editor dependent.

Only read on if you are interested in one more time to try and explain, it's not particularly relevant to the issue, except that we are conflating issues.

Kaocha or any test runner is a distracting example as the test runner as that test runner is not called from within Emacs. Running clj -M:test is a separate process to starting a relp via jack-in.
The test runner libraries are not used as the koacha test runner shows no sign of being called.
Why would you load in the kaocha libraries if your not going to use them in that process. The kaocha libraries are not required for Cider test runner, the built in test runner that looks as the tests inside the running repl.
If you did run kaocha by calling it's functions from the repl it would still just look at the filespace and not the deftests in the repl. Where would the output from kaocha go? So what is the point of this example.
Putting unused config into an alias is very inefficient.

@kirillsalykin
Copy link

kirillsalykin commented Dec 10, 2020

@jr0cket Please ignore the kaocha fully, the issue not related to the kaocha at all.

the problem is next:
I have the alias

:some-alias {
  :extra-deps {...}
  :extra-paths [...]
  :main-opts  ["-m" "my.main.here"]
}

Having this main-opts breaks the cider-jack-in because nrepl.cmdline is not the entry point anymore , the solution is to override :main-opts with ["-m" "nrepl.cmdline"] via appending cider alias.

@kirillsalykin
Copy link

kirillsalykin commented Dec 10, 2020

Offtopic

Why would you load in the kaocha libraries if your not going to use them in that process. The kaocha libraries are not required for Cider test runner

I know, but I want to load the test alias because it also adds other deps and paths.

@dpsutton
Copy link
Contributor Author

so will retire from this topic and switch to using cider-connect which seems to be a better workflow in the long run and I am not editor dependent.

Do you see anything in this proposal that will break a functionality of cider-jack-in? I'm confused because as I understand this, it will make cider-jack-in behave exactly the same except with the added benefit that any main opts in any included aliases will be trumped by the nrepl main. This should be 100% parity but with a bug fix. Why would you switch to cider-connect. If there's a problem we can certainly address it or not do this change if it introduces a bug or breaking change, I just don't see how that's possible.

Kaocha or any test runner is a distracting example as the test runner as that test runner is not called from within Emacs. Running clj -M:test is a separate process to starting a relp via jack-in.
The test runner libraries are not used as the koacha test runner shows no sign of being called.

Its hard to see how kaocha is a distracting example as its the exact use case that prompted this issue. Kaocha is used from the command line using the main args -m kaocha.runner from the alias:

/t/testing ❯❯❯ clj -A:test
WARNING: Use of :main-opts with -A is deprecated. Use -M instead.
[(.)]    # this is the output of kaocha's test running
1 tests, 1 assertions, 0 failures.  # this is a summary of all tests run by kaocha.
/t/testing ❯❯❯

And similarly run from a repl:

;; Connected to nREPL server - nrepl://localhost:57218
;;  Startup: /usr/local/bin/clojure -Sdeps '{:deps {nrepl {:mvn/version "0.8.2"} cider/cider-nrepl {:mvn/version "0.25.4"}}}' -A:test -m nrepl.cmdline --middleware '["cider.nrepl/cider-middleware"]'
user> (require '[kaocha.repl :as k])
nil
user> (k/run :unit)
[(.)]
1 tests, 1 assertions, 0 failures.
#:kaocha.result{:count 1, :pass 1, :error 0, :fail 0, :pending 0}
user> 

Why would you load in the kaocha libraries if your not going to use them in that process. The kaocha libraries are not required for Cider test runner, the built in test runner that looks as the tests inside the running repl.
If you did run kaocha by calling it's functions from the repl it would still just look at the filespace and not the deftests in the repl. Where would the output from kaocha go? So what is the point of this example.
Putting unused config into an alias is very inefficient.

It's hard to know how to respond to this. The libraries might not be used, but there's often some extra paths (as there was in this example) for tests to be on the classpath. Its true that kaocha is not required for the way CIDER can run tests, but that's not the only way to run tests. And if people have a large testing setup taking advantage of kaocha's ability to mark tests as unit, integration, etc., why should we force them to use CIDER's runner and test selector? The output from kaocha goes into the repl by default, both printing values directly and returning a value of #:kaocha.result{:count 1, :pass 1, :error 0, :fail 0, :pending 0}.

But fundamentally this issue is simply stated: CIDER cannot cider-jack-in if you include a profile that has main opts. I think this issue is worth solving, and I think there is a straightforward solution proffered in this issue. For cider-jack-in to work, the main must be nrepl.commandline. If we put this main opts into an alias in the -Sdeps {} form we manually inject, we can ensure this alias comes last with -A:cider/main (or whatever name we choose) and therefore the process will start with the main required for CIDER, regardless if any other aliases include main opts.

@bbatsov
Copy link
Member

bbatsov commented Dec 10, 2020

@dpsutton I like your idea, I think @jr0cket is referring mostly to the discussion in #2922 where he suggested a different way to structure the params for Clojure CLI.

But fundamentally this issue is simply stated: CIDER cannot cider-jack-in if you include a profile that has main opts. I think this issue is worth solving, and I think there is a straightforward solution proffered in this issue. For cider-jack-in to work, the main must be nrepl.commandline. If we put this main opts into an alias in the -Sdeps {} form we manually inject, we can ensure this alias comes last with -A:cider/main (or whatever name we choose) and therefore the process will start with the main required for CIDER, regardless if any other aliases include main opts.

Yep, very well summarized.

@FiV0
Copy link
Contributor

FiV0 commented Dec 28, 2020

I also just ran into this with https://github.com/cognitect-labs/test-runner. I think this is definitely worth solving.

@bbatsov
Copy link
Member

bbatsov commented Dec 28, 2020

No argument from me. I'm assuming @dpsutton will tackle this when he has time for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants