babashka / bbin

Install any Babashka script or project with one command
MIT License
139 stars 9 forks source link

`bbin ls` fails if the `bbin` bin has a 0-length file #88

Closed jdjohnston closed 2 months ago

jdjohnston commented 3 months ago

Why would the bin have a 0-length file? I don't know about others, but I use 0-length files, .e.g. .notar, as flags in my dirs.

It's an easy fix. Add something like (> (fs/size %) 0) to the file filter in load-scripts (currently line 1510 in bbin). The change worked fine for me on a local copy of bbin.

Thanks for this cool util!

borkdude commented 3 months ago

What's the error your getting, just out of curiosity? Can you paste a stack trace?

jdjohnston commented 3 months ago

@borkdude my laptop is currently offline, but I can provide a verbal stack trace.

str/split-lines in parse-script errs with a NullPointerException because read-header returns a nil for a 0-length file. read-header returns a nil because .read returns -1, which isn't a nat-int?.

Arguably, a cleaner fix might be to change read-header from (when (nat-int? n) ...) to (if (nat-int? n) ... ""), so that it always returns a string, even for a 0-length file.

BTW, I wish the JVM Clojure stack traces were as user-friendly as Babashka's. Well done. :)

borkdude commented 3 months ago

@jdjohnston Nice! Since you analyzed the root cause, perhaps you're willing to put a PR together? If not, I'll get it sooner or later!

borkdude commented 3 months ago

I took a look at this issue and don't see why read-header has the (when (nat-int? n) ..) condition. You can create a string like this: (String. (byte-array (range 1000)) 0 0) which is just an empty string. Can we remove the condition @rads?

rads commented 3 months ago

That sounds good to me. 👍

jdjohnston commented 3 months ago

Don't forget, .read returns -1 when the file is 0-length. (Conflating 0-length with EOF?)

On Fri, Aug 23, 2024, 14:30 Radford Smith @.***> wrote:

That sounds good to me. 👍

— Reply to this email directly, view it on GitHub https://github.com/babashka/bbin/issues/88#issuecomment-2307603716, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD4V3RNQ4BEAOU6QNZBLWXLZS55UNAVCNFSM6AAAAABMVTGI3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBXGYYDGNZRGY . You are receiving this because you were mentioned.Message ID: @.***>

jdjohnston commented 3 months ago

@borkdude , @rads How about replace when expression with (String. buffer 0 (max 0 n)) ?

n will be one of three values:

borkdude commented 2 months ago

Sorry, I got a little overwhelmed with other issues. Feel free to submit a PR if you'd like

borkdude commented 2 months ago

@jdjohnston Perhaps you're able to test now that I've merged this

jdjohnston commented 2 months ago

@borkdude Works for me after I remembeed to bb gen-script. Your fix is different than either of my suggestions :), but it works fine.

I suppose gen-script will be executed for the next release. Am I understanding the workflow correctly?

BTW, I do understand it is better to fix a root-cause than to "band-aid" a symptom. Thanks for the reminder. Next time, feel free to be more blunt. It won't hurt my feelings. :)

borkdude commented 2 months ago

Returning an empty string was one of your suggestions right?

borkdude commented 2 months ago

Published as 0.2.4

jdjohnston commented 2 months ago

@borkdude I was largely joking. Absolutely no criticism intended. There are probably even more ways to return an empty string. If I truly wanted it done in a certain way, I should have made more effort to generate a PR.

It works. That's the important part.