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

-X/-T usage affected by System/exit #261

Closed
seancorfield opened this issue Sep 9, 2024 · 10 comments
Closed

-X/-T usage affected by System/exit #261

seancorfield opened this issue Sep 9, 2024 · 10 comments
Assignees

Comments

@seancorfield
Copy link
Contributor

When used as a tool (-X/-T usage), the expected "exit" behavior is to just return a value or throw an exception, rather than calling System/exit.

I suspect this is why the following does not work properly on Clojure 1.12.0:

user=> (invoke-tool {:debug true :preserve-envelope true :tool-name "antq" :fn 'outdated :args {:clojure.exec/out :capture}})
args [outdated #:clojure.exec{:out :capture, :invoke :fn}]
Invoking:  clojure -Tantq -
Execution error (ExceptionInfo) at clojure.tools.deps.interop/invoke-tool (interop.clj:83).
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.

What should happen here is that the execution "envelope" containing stdout should be returned, but instead no envelope is captured and the presence of stderr output (from SLF4J) leads invoke-tool to assume the tool has failed.

I believe that if antq returned a value (even nil) for successful tool execution, or threw an exception for unsuccessful tool execution, then it would work correctly with 1.12's invoke-tool function.

@liquidz
Copy link
Owner

liquidz commented Sep 9, 2024

@seancorfield Thanks for your reporting! I'll have a look.

@liquidz liquidz self-assigned this Sep 9, 2024
liquidz added a commit that referenced this issue Sep 20, 2024
@liquidz
Copy link
Owner

liquidz commented Sep 20, 2024

@seancorfield I've fixed in fix-261 branch.
Coud you try?

@seancorfield
Copy link
Contributor Author

That certainly seems to fix the installed tool usage, yes! Thank you!

@liquidz
Copy link
Owner

liquidz commented Sep 20, 2024

@seancorfield Thanks for your confirmation!

@liquidz
Copy link
Owner

liquidz commented Sep 20, 2024

@seancorfield Just released v2.9.1232 :)

@liquidz liquidz closed this as completed Sep 20, 2024
@dpassen
Copy link
Contributor

dpassen commented Sep 21, 2024

This broke running via '-X'

$ clj -X:antq
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.
[##################################################] 7/7
| :file    | :name               | :current | :latest |
|----------+---------------------+----------+---------|
| deps.edn | org.clojure/clojure | 1.11.4   | 1.12.0  |

Available changes:
- https://github.com/clojure/clojure/blob/clojure-1.12.0/changes.md
Execution error (ExceptionInfo) at antq.tool/outdated$fn (tool.clj:56).
Exited

Full report at:
/var/folders/l2/4c_1v4jj7kj9sfhwjmplz2s80000gn/T/clojure-14409212056517983648.edn

with the alias defined as:

:antq
{:extra-deps
 {io.github.liquidz/antq
  {:git/tag "2.9.1232"
   :git/sha "c1d0f9e"}}
 :exec-fn antq.tool/outdated}

@seancorfield
Copy link
Contributor Author

I'll argue that's not broken: that's the behavior it should have with with -X for "failed run" -- throw an exception. That's idiomatic.

You could definitely argue for a new option that doesn't treat outdated deps as an error.

Or you can use -M -m and rely on the exit status.

@seancorfield
Copy link
Contributor Author

(as an example, see Cognitect's own test-runner when run with -X it throws an exception if any tests fail; with -M -m it exits with a non-zero status instead)

@frou
Copy link

frou commented Sep 27, 2024

clojure -Tantq outdated exiting with status 1 is completely understandable, but Execution error and the last few lines of the output pointing at a file with a stack trace is goofy. The majority of users (incl. me) are probably going to think that means the tool is broken and it's time to file a bug report.

@seancorfield
Copy link
Contributor Author

The README should be updated to clarify the error case -- but this is how -X / -T tooling is generally supposed to work, so that it interops properly with build.clj stuff and Clojure 1.12's invoke-tool. The Execution error and stack trace is coming from the Clojure CLI in this case but would be programmatic in those other cases and you would try/catch in your code.

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

4 participants