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

Node LTS version 10 support (update to Lumo 1.9.0) #113

Open
ghost opened this issue Nov 28, 2018 · 7 comments
Open

Node LTS version 10 support (update to Lumo 1.9.0) #113

ghost opened this issue Nov 28, 2018 · 7 comments
Labels

Comments

@ghost
Copy link

ghost commented Nov 28, 2018

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
Copy link
Owner

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
Copy link
Owner

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.

@dundalek dundalek changed the title Node LTS version 10 support Node LTS version 10 support (update to Lumo 1.9.0) Nov 28, 2018
@dundalek dundalek added the lumo label Nov 28, 2018
@s-ol
Copy link

s-ol commented Mar 20, 2019

  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
Copy link
Owner

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
Copy link

s-ol commented Apr 9, 2019

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

@dundalek
Copy link
Owner

dundalek commented Apr 9, 2019

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

@s-ol
Copy link

s-ol commented Apr 10, 2019

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

2 participants