dundalek/closh

Node LTS version 10 support (update to Lumo 1.9.0)

Opened this issue · 7 comments

Deleted user commented 
ERROR: Closh requires node version 9, but your version is v10.13.0.

Would it be possible to support Node version 10, the current LTS version?

dundalek commented

I plan to do that. Latest Lumo is built on Node 10, but there I some incompatibilities with Closh I need to fix first.

dundalek commented

I did a bit of digging into it. Here is the report of found issues:

1) File expansion is broken

File expansion does not work, for example "ls *" which internally calls (expand "*") will not return the files in the directory. There are many test failures which most seem to be related to this:

Ran 16 tests containing 188 assertions.
14 failures, 1 errors.

I was able to track this down and it is caused by broken fs module: anmonteiro/lumo#453

2) Can't require builtin node modules

Refer to this: anmonteiro/lumo#451

This is a minor issue. There exist a trivial workaround to this by replacing occurrences of require with js/require across the code base.

3) load-file and a workaround

load-file does not work for eval (see anmonteiro/lumo#449) so I use a workaround of by defining:

(defn load-file [f]
  (lumo.repl/execute-path f {})))

However execute-path is private and latest clojurescript stricter checks so following warning gets printed out which adds noise:

WARNING: var: lumo.repl/execute-path is not public at line 16

4) setenv warnings

Calling setenv now prints a warning. Which is strange since setenv is defined to have variable number of arguments (defn setenv [& args] ...).

$ (setenv "XXX" "abc")
WARNING: Wrong number of args (2) passed to closh.zero.builtin/setenv at line 1
("abc")

However it works correctly as verified with:

$ bash -c "echo $XXX"
abc

Summary

Issues 2), 3) and 4) are possible to be fixed. In the worst case we could just disable warnings during closh startup. However 1) is the blocker, we cannot move forward due to this. And unfortunately I have no idea at the moment what the fix might be.

s-ol commented
  1. only affects the directory lumo was started from, so it would be possible to start closh in a wrapper like
work=`pwd`
cd `mktemp -d`
exec closh

and execute (process.chdir workdir) first thing inside lumo.

dundalek commented

Thanks for the info. This is a great finding, I think it is a great starting point to locate the bug in lumo.

I would prefer to have the bug fixed in upstream lumo rather than adding more hacks into closh. There are enough hacks as it is 😅

s-ol commented

@dundalek looks like upstream is fixed, but a release is still pending

dundalek commented

Yeah, it looks I it coming soon: https://twitter.com/_anmonteiro/status/1115630062995374080. I will do an update once it is out.