Skip to content
This repository was archived by the owner on Nov 21, 2020. It is now read-only.

Conversation

@cedroux
Copy link
Contributor

@cedroux cedroux commented Dec 4, 2017

In some occurences, when reading the stdout buffer, the output can contain multiple lines at once.
I changed the conditions to take that case into account.

This PR will probably solve #14 and #18,

@kachkaev
Copy link
Owner

kachkaev commented Dec 4, 2017

Thanks for your contribution @cedroux! WHich operating systems did you test the new solution on? I originally wrote php-r on macos, just curious if PR can break some things in addition to fixing others.

@cedroux
Copy link
Contributor Author

cedroux commented Dec 4, 2017

I tested it on a Vagrant box running on Ubuntu Linux (Laravel Homestead)

$ uname -a
Linux homestead 4.4.0-92-generic #115-Ubuntu SMP Thu Aug 10 09:04:33 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

Edit: With PHP 7.1

@cedroux
Copy link
Contributor Author

cedroux commented Dec 4, 2017

After some more tests, I can tell this PR may break some things reveal some other unrelated bugs.

For instance, before my PR the following R script:

print('Hello World! 1')
print('Hello World! 2')
print('Hello World! 3')

Would output:

print('Hello World! 1')\n
[1] "Hello World! 1"\n
[1] "Hello World! 2"\n
[1] "Hello World! 3"

Instead of

[1] "Hello World! 1"\n
[1] "Hello World! 2"\n
[1] "Hello World! 3"

To hide the first line I had to do some black magic with PHP_EOL (\n alias):

$rProcess->write(PHP_EOL . PHP_EOL . $script . PHP_EOL);

Now with my PR, this trick doesn't work anymore and the first line is always returned by the getAllOutput() method.
It is another issue, but my PR reveals it more.

Edit: And in some case, the two first lines are not displayed. There is some kind of i/o mangling that I'm not sure to understand here.

@cedroux
Copy link
Contributor Author

cedroux commented Dec 7, 2017

Ok, with the last two commits I think I nailed it.

59b3d20: The options() command was passed before trying to read the header, hence the strange and inconsistent behaviour.
ace7310: Some times (usually on the first line) a prompt (> \n) was read before receiving the inputted line. Thats why the app was considering the inputted R line as a real output.

@kachkaev
Copy link
Owner

kachkaev commented Dec 7, 2017

Good to see new comments @cedroux! So am I right that outputting this is no longer the case?

print('Hello World! 1')\n
[1] "Hello World! 1"\n
[1] "Hello World! 2"\n
[1] "Hello World! 3"

@cedroux
Copy link
Contributor Author

cedroux commented Dec 7, 2017

Almost, the exact current output is now:

\n
[1] "Hello World! 1"\n
[1] "Hello World! 2"\n
[1] "Hello World! 3"

Maybe we should check if $currentCommandInput is not null before inserting it in $this->inputLog (here)

@kachkaev
Copy link
Owner

kachkaev commented Dec 7, 2017

That worries me a bit, because other users may face issues that'll be really hard to investigate. Do you think you could find a solution that would be 100% backwards-compatible?

@cedroux
Copy link
Contributor Author

cedroux commented Dec 7, 2017

Oh OK, I just understood the problem here, and it was my fault, sorry!
I totally forgot that I was appending a setwd() command in front of all of my R scripts.

All of the commands that don't have an output will just append a new \n in the final output.

Example:

var1 = TRUE
var2 = TRUE
var3 = TRUE
print('Hello World!')
var4 = TRUE
var5 = TRUE
var6 = TRUE

$rProcess->getAllOutput() will return

\n
\n
\n
[1] "Hello World!"\n
\n
\n

It was like this before my PR but I forgot about it. Is it the expected behaviour?

@kachkaev
Copy link
Owner

kachkaev commented Dec 7, 2017

If I remember correctly, yes it is. Will you be happy if this PR is merged now?

@cedroux
Copy link
Contributor Author

cedroux commented Dec 8, 2017

Yes it is OK for me.
I tested it on some huge R scripts and it works flawlessly now. No more fread() locks or unwanted output (except for the multiples \nthat I'm filtering afterhand).

@kachkaev kachkaev merged commit d8daf71 into kachkaev:master Dec 8, 2017
@kachkaev
Copy link
Owner

kachkaev commented Dec 8, 2017

Thanks again @cedroux! Your changes are in v1.0.1 now.

@cedroux cedroux deleted the patch-1 branch December 8, 2017 10:30
@mamoralesri
Copy link

I'm using version 1.0.1 (2017-12-08) of the php-r library on linux servers (16.04 and 18.04). On both servers it happens that sometimes the page is waiting for the server (waiting localhost...). If the load is interrupted and reloaded, the page loads correctly. After several successful recharges, the page does not reload again, it remains indefinitely waiting for the server (waiting localhost...). This happens in a cyclical way.

What do you suggest to avoid this situation?

@kachkaev
Copy link
Owner

Hi @mamoralesri! I'm sorry, but I won't be able to help here. Haven't used PHP or R for over two years, mainly Node.js now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants