Fork me on GitHub
#clj-kondo
<
2019-11-02
>
Filipe Silva10:11:19

@snoe @borkdude heya, I wanted to talk to you about making a npm distro of clojure-lsp in the same way I made one for clj-kondo

Filipe Silva10:11:45

so the clj-kondo one felt like it made sense because it would be ran standalone in non-server mode

Filipe Silva10:11:55

does clojure-lsp have a non-server mode?

Filipe Silva10:11:37

if it doesn't, I wonder if it makes sense to have a npm distribution... because it feels like something that actually runs as a editor extension instead

Filipe Silva10:11:58

and from looking at the clj-kondo editor extension, it doesn't seem to need a npm distro

Filipe Silva11:11:44

should the focus instead be to provide a clojure-lsp vscode extension similar to the clj-kondo one? would it make sense for them to be the same extension even?

borkdude12:11:53

@filipematossilva Let's back up a bit. The clj-kondo.lsp + vscode extension are available from the VSCode marketplace. What would be the added benefit of an npm package?

Filipe Silva12:11:25

yes I suppose that is the main question

Filipe Silva12:11:53

to give npm consumers a cross platform way to run closure-lsp

Filipe Silva12:11:00

not sure who those consumers would be though

Filipe Silva12:11:33

relevant to note that the closure-lsp vscode extension doesn't work on windows (or didn't, last I checked)

borkdude12:11:52

it should work on Windows as it's just running a JVM, nothing native. @pez checked that it worked on Windows

borkdude12:11:46

I could see distributing clj-kondo.lsp as a library on clojars being useful, so others can build extensions for other editors if they want. That's something I'm considering

borkdude12:11:36

the whole point of clj-kondo.lsp was Windows actually, since the binary already worked on other platforms

borkdude12:11:11

so maybe you could re-test it @filipematossilva on Windows?

borkdude12:11:43

it does require a java installation to be available on the path

Filipe Silva12:11:38

about a week ago I tried opening the extension and the changelog said it didn't work on windows yet

borkdude12:11:31

which changelog?

Filipe Silva12:11:59

the clojure-lsp vscode extension one

Filipe Silva12:11:10

trying to find it again, but having trouble... sec

borkdude12:11:11

can you make a screenshot, so I know what you're seeing?

borkdude12:11:34

oh clojure-lsp, sorry, I thought you meant clj-kondo.lsp!

borkdude12:11:45

the VSCode extension borkdude.clj-kondo should work on Windows

Filipe Silva12:11:53

ah no, the clj-kondo vs code extension is working on windows already

Filipe Silva12:11:40

it's the clojure-lsp one that doesn't

borkdude12:11:03

sorry for the confusion

pez12:11:08

clj-kondo.lsp solves more than just the Windows case. It also lets makes the story for vscode clojure linting be the beautifully simple: “Install the cljs-kondo extension”, or even “Install Calva. Oh, you already did that? You have linting.“.

borkdude13:11:58

@lee I have a debug-lock-file branch now and added the suggested println

borkdude13:11:12

this works:

$ ./clj-kondo --lint - <<< '(inc)'
exists? false
can-read? false
<stdin>:1:1: error: clojure.core/inc is called with 0 args but expects 1

borkdude13:11:29

but with the large classpath I don't even see those printlns:

$ ./clj-kondo --lint "$(lein classpath)"  > /tmp/log.txt
java.io.FileNotFoundException: /private/tmp/lock-kondo/.clj-kondo/.cache/2019.10.27-SNAPSHOT/lock

borkdude13:11:22

going to add some moar debugging

lread13:11:33

you might consider adding (flush) after your printlns

lread13:11:23

just to be sure your output is is flushed even on exception

borkdude14:11:43

tried that 🙂

borkdude14:11:47

(see branch)

borkdude14:11:38

but println already calls flush itself too

borkdude15:11:33

@lee @sogaiu found it! it has to do with not closing the JarFile objects 🙂

👍 8
borkdude15:11:52

strangely completely elsewhere in the code than where it was reported, maybe due to laziness or something

borkdude15:11:50

so it had to do with not closing something after all, thanks for the hint @lee

borkdude15:11:40

@lee could you try that one too since you're on Mac?

borkdude15:11:59

@rickmoynihan It was the problem with: > I’ve mkdir .clj-kondo and when running `clj-kondo --lint “$(lein classpath)” I get the error:

borkdude15:11:46

I'm so happy this weird bug is now solved (at least on my machine, fingers crossed). Thanks a lot for collaborating.

🎉 8
rickmoynihan15:11:46

@borkdude 🍾 great news. So was the issue laziness holding jar files open for too long? Also just tried to recreate the issue but no longer can… weird.

borkdude15:11:33

Well, because of laziness you can't close a resources, because then you will close it too soon. So I had to make reading sources from a jar strict + close it after reading it.

rickmoynihan15:11:17

yeah, I’ve done that myself but with zip files in the past… laziness and resources don’t mix! 😀

borkdude15:11:33

exactly the same problem 🙂

borkdude15:11:02

@rickmoynihan It depends on the amount of dependencies. Try this project.clj: https://github.com/borkdude/clj-kondo/files/3799357/project.clj.zip

borkdude15:11:39

I might try a transducer approach in the future, but for now this works well enough

rickmoynihan15:11:08

yeah a transducer might make sense — but probably not worth the effort porting

borkdude15:11:47

I guess reading all the sources from a jar in memory at once isn't adding up to enough making it worth porting

borkdude15:11:22

but a fun exercise, so low priority

rickmoynihan15:11:58

oh it’s slurping… I guess most clojure projects are pretty small, even with all their transitive deps.

borkdude15:11:43

yeah, it's slurp. could make that into a reader-type thing, but I guess even with this classpath size it doesn't even add up

rickmoynihan15:11:48

but yeah might be worth porting one day after all… though I don’t see myself maintaining 1m lines of clojure in a project for a while.

borkdude15:11:39

also linting the classpath is a thing people do once every so often, the most common use case is linting one file at a time

lread16:11:34

giving it a go on macOS now...

lread16:11:45

(congrats btw!)

lread16:11:52

looks good! also compared output from both and looks good:

> diff native.out jvm.out
4485c4485
< linting took 23197ms, errors: 1582, warnings: 2902
---
> linting took 16499ms, errors: 1582, warnings: 2902

borkdude16:11:21

thanks 🙂

lread16:11:32

my pleasure

rickmoynihan16:11:22

weird that project.clj seems to work for me…

borkdude16:11:38

with the old binary on MacOS?

rickmoynihan16:11:17

well I’m not sure it’s the same binary I used when I first reported the issue; but I’ve not updated to the binary you posted today.

borkdude16:11:10

ok well, thanks for testing. I have two different macOS machines confirming that it's fixed now 🙂

borkdude16:11:59

@lee @sogaiu I now updated the GraalVM issue using a pure Java example: https://github.com/oracle/graal/issues/1804#issue-516316985

borkdude16:11:08

Maybe it would be fun if you could also try it

borkdude17:11:45

To see how it behaves on your machines

sogaiu17:11:19

@borkdude i got:

Exception in thread "main" java.lang.NullPointerException
	at LockRepro.listFiles(LockRepro.java:13)
	at LockRepro.listFiles(LockRepro.java:25)
	at LockRepro.listFiles(LockRepro.java:25)
	at LockRepro.listFiles(LockRepro.java:25)
	at LockRepro.listFiles(LockRepro.java:25)
	at LockRepro.listFiles(LockRepro.java:25)
	at LockRepro.main(LockRepro.java:33)

borkdude17:11:20

(it also shows how easy it is to create a binary from a single Java file)

lread17:11:20

java LockRepro.java should be javac LockRepro.java

sogaiu17:11:33

yes, i had to do what lread said too

sogaiu17:11:08

neat about creating a binary so easily

borkdude17:11:41

the resulting binary is 2.6mb

sogaiu17:11:56

that's about how big it is here too

borkdude17:11:22

@sogaiu since you're getting the exception now, my hunge is that the maximum number of open files for a process on macOS is lower than on linux?

sogaiu17:11:51

that sounds familiar -- and like what i hear in #shadow-cljs iirc

lread17:11:02

got similar result to yours for native:

> ./lockrepro | wc -l
Exception in thread "main" java.lang.NullPointerException
	at LockRepro.listFiles(LockRepro.java:13)
	at LockRepro.listFiles(LockRepro.java:25)
	at LockRepro.listFiles(LockRepro.java:25)
	at LockRepro.listFiles(LockRepro.java:25)
	at LockRepro.listFiles(LockRepro.java:25)
	at LockRepro.listFiles(LockRepro.java:25)
	at LockRepro.main(LockRepro.java:33)
     253

lread17:11:32

670 jars created for jvm for me.

borkdude17:11:37

what do you guys see when executing: $ ulimit -n

borkdude17:11:56

ah that explains it then probably

lread17:11:08

error sucks though

lread17:11:23

or at least misleads

borkdude17:11:28

indeed it does

sogaiu17:11:13

i suppose you might test by changing your limits?

lread17:11:26

native succeeds after ulimit -n 1024

sogaiu17:11:35

so quick 🙂

borkdude17:11:33

tl;dr: error caused by too many open files, but the error thrown is misleading

lread17:11:54

so it does not apply to jvm version because... ?

sogaiu17:11:02

iiuc, graal uses a patched jdk -- may be that's related?

sogaiu17:11:26

no, that's probably not it

sogaiu17:11:09

native-image binaries are diff beasts than the actual jvm

borkdude17:11:12

I was considering to do parallel linting. I can change mapcat into pmap + cat. But maybe I would also run into "too many open files" with this 🙂

borkdude17:11:49

probably not though, 8 threads would just mean 8 open files

lread17:11:29

a process must be able to set its own limit no? maybe through setrlimit... maybe there is a graal option for that?

lread17:11:05

gotta run some errands, later! 👋

👋 8