plasma-umass / browsix

Browsix is a Unix-like operating system for the browser.
Other
3.16k stars 182 forks source link

xargs: implementation #24

Closed rvolosatovs closed 8 years ago

rvolosatovs commented 8 years ago

Some of the options still do not work in the shell, probably because of funcionality lacking. The binary in /fs/usr/bin/ works as expected.

bpowers commented 8 years ago

cool! this looks very promising, I'll do a detailed review in the morning (US Eastern timezone) tomorrow!

bpowers commented 8 years ago

yea, indeed, the test you have:

echo 1,12,123,12,1,1234 | xargs -n 2 -s 13 -d , -x

runs under GNU xargs:

1 12
123 12
1 1234

runs under node for me, but produces the error:

cannot fit single argument within argument list size limit

And in the browser, it causes the worker to allocate over a gig of RAM before Chrome kills the process.

I think this is because in browser-node process.exit returns, whereas it doesn't return in real-node. I've thought about making process.exit throw an exception to interrupt normal control flow, but this results in a number of uncaught exception errors in the Chrome debugger.

Sticking in some returns after process.exit gives me some better error messages.

rvolosatovs commented 8 years ago

The default command executed is not 'echo', as was implemented initially, but '/usr/bin/echo' (after the workarounds for Browsix shell). with 'echo 1,12,123,12,1,1234 | xargs -n 2 -s 13 -d , -x /usr/bin/echo' GNU xargs provide the exact same result (cannot fit single argument within argument list size limit) Browsix shell hangs when just 'echo' is used '-n' seems to be the only option hanging the shell, will try to fix now

rvolosatovs commented 8 years ago

The process is able to spawn only 1 more process. The command fails, when there is more than 1 query to be passed. (TypeError: this.files[i] is undefined in Kernel.js:1293 - method Task() ). Works as expected if only 1 command is there to be run. As it works in node, I assume the kernel is the reason for that. Could be that the parent process is determined wrong?

rvolosatovs commented 8 years ago

@bpowers Line 905 in kernel.js - this.files[i].ref() , what is the purpose of it? Commenting this line out solves the issue. Looks like this.files is set to undefined after executing that line (and so execution on each next iteration fails) In my experience, the shell still hangs in Firefox occasionally, but works as expected in Chromium

rvolosatovs commented 8 years ago

http://i.imgur.com/PJT30Bq.png A screenshot showing execution of the command in Chromium

rvolosatovs commented 8 years ago

Indeed, it fixes the issue. The default command run is executed as '/usr/bin/echo', not 'echo', so the '-s' option, in my opinion, works as it should. The default command should be changed to just 'echo', when that behaviour is supported.

bpowers commented 8 years ago

@flat1101 I just pushed a commit that gets echo working. This now runs:

$ echo 1,12,123,12,1,1234 | xargs -n 2 -s 13 -d , -x
1 12
123 12
1 1234

Which matches what GNU xargs gives on my system (but is different from what the unit test is expecting). So I think we're close! Could you merge from the master branch and take a look?

rvolosatovs commented 8 years ago

Will do! Sorry, but I still can't manage to run the tests on my own system, but the output should match

bpowers commented 8 years ago

@flat1101 this was great! I made one small fix to get the last test passing, and merged.