feat: implement io.popen #88

TorchedSammy · 2022-05-25T22:29:02Z

as of writing, the pr only adds the popen function; iolib.File is untouched.
is there anything else i need to add here, like tests? also how should i actually handle file options, or is it fine as-is?

closes #7

arnodel

That seems almost it. Apart from the mode validation I think there is another issue to figure out: what happens when the file is closed? A simple test in lua 5.4 gives this:

> f = io.popen("echo foo")
> f
file (0x21021ac10)
> f:close()
true	exit	0
> f
file (closed)

What happens in Golua? Given there is no real file handle backing up the File instance I think things may go wrong here.

A few tests would be good also, best if they could be cross-platform.

lib/iolib/iolib.go

codecov · 2022-05-26T07:53:34Z

Codecov Report

Base: 89.05% // Head: 88.82% // Decreases project coverage by -0.23% ⚠️

Coverage data is based on head (cd624ec) compared to base (4f77264).
Patch coverage: 67.52% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           lua5.4      #88      +/-   ##
==========================================
- Coverage   89.05%   88.82%   -0.24%     
==========================================
  Files         104      104              
  Lines       11122    11233     +111     
==========================================
+ Hits         9905     9978      +73     
- Misses        921      949      +28     
- Partials      296      306      +10

Impacted Files	Coverage Δ
lib/iolib/file.go	`80.17% <66.66%> (-1.31%)`	⬇️
lib/iolib/iolib.go	`87.42% <67.64%> (-5.00%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TorchedSammy · 2022-05-26T12:47:04Z

an idea for handling file operations on popen is that i could add the cmd to iolib.File and add specific functions for that. what do you think?

also how do i improve coverage... by writing the lua tests?

arnodel · 2022-05-26T13:26:15Z

an idea for handling file operations on popen is that i could add the cmd to iolib.File and add specific functions for that. what do you think?

It depends on the behaviour that popen implements I think. It's not really specified in the Lua docs afaict so it might be a matter of testing it in Lua (or looking at the code). Go's exec.Cmd.StdoutPipe() actually returns an io.ReadCloser instance so it might be enough to call Close() on that when closing the Lua file (simplest way might be to check if the the reader implements io.Closer when there is no file to close - and do it with the writer in the case we are writing into the process instead of reading from it). The problem is that I don't know if that leads to behaviour consistent with the C Lua implementation. It could be good enough as a first go anyway!

also how do i improve coverage... by writing the lua tests?

Yes :). I am not very familiar with windows but perhaps the windows shell implements echo and cat >file which would allow testing both reading from and writing to a process?

TorchedSammy · 2022-05-26T13:29:51Z

Go's exec.Cmd.StdoutPipe() actually returns an io.ReadCloser instance so it might be enough to call Close() on that when closing the Lua file

close on popen returns the process' status (same returns as os.execute) so i don't think it'll be enough

arnodel · 2022-05-26T13:56:49Z

Go's exec.Cmd.StdoutPipe() actually returns an io.ReadCloser instance so it might be enough to call Close() on that when closing the Lua file

close on popen returns the process' status (same returns as os.execute) so i don't think it'll be enough

Yeah I just had a quick look at C Lua, Lua file objects contain a callback for closing the file. In the case of files opened by popen, the callback calls pclose(). I think the equivalent in Go is going to be to call reader.Close() (or writer.Close()) and then to do command.Wait(). You could copy the Lua approach and add a (optional?) callback to the File data structure that does closing.

TorchedSammy · 2022-05-26T15:47:18Z

there is another issue to figure out: what happens when the file is closed? [...]
Given there is no real file handle backing up the File instance I think things may go wrong here.

fixed. tested in hilbish

~/Files/Projects/Hilbish master*
∆ f = io.popen 'sleep 5' 
~/Files/Projects/Hilbish master*
∆ print(f:close())
true	exit	0
~/Files/Projects/Hilbish master*
∆ f = io.popen 'false'
~/Files/Projects/Hilbish master*
∆ print(f:close()) 
nil	exit	1
~/Files/Projects/Hilbish master*
∆ f = io.popen 'sleep 15'
~/Files/Projects/Hilbish master*
∆ print(f:close())
nil	signal	15

TorchedSammy · 2022-05-26T15:53:26Z

now just the other functions, and tests

TorchedSammy · 2022-05-26T16:54:12Z

for tests i just copy pasted already available ones (but only 2 suites)
everything should be good now (except for setvbuf - not sure what to do with that)

TorchedSammy · 2022-05-26T20:07:49Z

ok i'm not sure why tests are failing and how wait is blocking

arnodel · 2022-05-26T20:10:17Z

ok i'm not sure why tests are failing and how wait is blocking

I think the test

    local f = io.popen("cat files/iotest.txt")

should be

    local f = io.popen("cat files/iotest.txt", "w")

You want to write to the process, not read from it. I might have some time to look at it tomorrow.

TorchedSammy · 2022-05-26T20:23:26Z

no it's the popen write test (local f = io.popen("cat > files/popenwrite.txt", "w"))
looks like gopher-lua has the same problem, but c lua doesn't

arnodel · 2022-05-27T08:20:20Z

no it's the popen write test (local f = io.popen("cat > files/popenwrite.txt", "w")) looks like gopher-lua has the same problem, but c lua doesn't

As a sanity check I wrote this little Go program:

func main() {
	cmd := exec.Command("/bin/sh", "-c", "cat > foo.txt")
	stdin, _ := cmd.StdinPipe()
	fmt.Println("Starting cat...")
	cmd.Start()
	go func() {
		fmt.Println("Sending greeting...")
		fmt.Fprintln(stdin, "Hello from test")
		fmt.Println("Waiting 5s...")
		time.Sleep(5 * time.Second)
		fmt.Println("Closing stdin...")
		stdin.Close()
	}()
	fmt.Println("Waiting for cat to exit...")
	cmd.Wait()
	fmt.Println("Done")
}

That terminates fine, and suggests that cmd.Wait() should not block provided the stdin pipe was closed previously. I probably don't have time today sadly.

…ite mode

TorchedSammy · 2022-05-28T01:34:28Z

weirdly, not using a type assert and just storing the stdout/stdin pipe makes it work from my side. now we should be good

lib/iolib/lua/iolib.lua

lib/iolib/file.go

Co-authored-by: Arnaud Delobelle <arnodel@gmail.com>

…d stdin/out

TorchedSammy · 2022-05-31T16:05:51Z

anything else to fix/improve? i'm not sure how to improve the coverage; what else to test

arnodel · 2022-05-31T17:12:47Z

anything else to fix/improve? i'm not sure how to improve the coverage; what else to test

If you could just disable the failing test on windows, then I'll merge it. Don't worry about test coverage atm

TorchedSammy · 2022-05-31T17:49:31Z

If you could just disable the failing test on windows,

oh how do i do that

TorchedSammy · 2022-05-31T22:49:36Z

does that work?

arnodel · 2022-06-01T08:42:20Z

does that work?

Sadly that doesn't work because the expected results are not conditional. I should have some time tomorrow so hopefully I can figure out something to help with that.

arnodel · 2022-06-02T14:30:39Z

does that work?

Sadly that doesn't work because the expected results are not conditional. I should have some time tomorrow so hopefully I can figure out something to help with that.

I added a feature to help with this: 41f7e93

You should be able to put the tests that fail on windows in a file e.g. non_windows_popen.lua and set the first line to be

-- tags: !windows

It will only run this lua test file on non-windows platforms

HTH

Edit: this commit is on the default branch (lua5.4)

Edit 2: the working commit is 8cf7d84

This reverts commit 8c75e6b.

…indows tests

TorchedSammy · 2022-06-03T00:38:30Z

like that?

arnodel

Yes that looks good, thank you. Windows tests are still failing though (see comments). I do not have a windows environment to test in, so I don't have any suggestions why they are not working. If you can't debug the issue either and you don't need a windows implementation we could always disable popen on windows for now? I think it would be better than to have a broken implementation.

arnodel · 2022-06-03T08:16:58Z

lib/iolib/lua/popen_windows.lua

+    --> ~^false\t.*invalid format
+
+
+    for line in f:lines() do


Looks like this is not finding any lines in windows (see e.g. test failure at https://github.com/arnodel/golua/runs/6722468583?check_suite_focus=true#step:4:35)

how does that even make sense

TorchedSammy · 2022-06-04T22:26:48Z

i wanna have popen on windows at least, but this is infuriating

edit: i do have a windows install but it is excruciatingly slow and i would rather not use it

arnodel · 2022-06-06T12:05:23Z

i wanna have popen on windows at least, but this is infuriating

edit: i do have a windows install but it is excruciatingly slow and i would rather not use it

I can try to find a way forward, but it probably won't be very quick as it means getting a windows environment.

TorchedSammy · 2022-06-06T12:08:56Z

I can try to find a way forward, but it probably won't be very quick as it means getting a windows environment.

that's fine, i won't be able to do much because of school finals

TorchedSammy · 2022-06-21T00:22:27Z

hello?

arnodel · 2022-06-22T10:56:52Z

Sorry I haven't got round to provisioning a windows machine yet. Hopefully I get some time soon.

…

On Tue, 21 Jun 2022 at 01:22, sammyette ***@***.***> wrote: hello? — Reply to this email directly, view it on GitHub <#88 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMJKONPFLIZUYMDYVGKWBDVQEDM3ANCNFSM5W6Z4B4Q> . You are receiving this because you commented.Message ID: ***@***.***>

arnodel · 2022-07-11T10:06:55Z

Update: I only have an apple silicon mac at the moment and it appears to be tricky to run windows on it - so far no luck so I'll need to get another way to have access.

TorchedSammy · 2022-08-17T04:03:21Z

been a while huh

i'm on windows myself now and it seems to actually be decent so i can try testing to fix the issue

TorchedSammy · 2022-08-17T17:48:24Z

the path needed a backward slash

yeah that makes sense
with that all the iolib tests pass so this can be merged !!
~~i'll make the fix later (i don't have my gpg key on windows)~~

TorchedSammy · 2022-12-13T19:21:43Z

ya good? didnt want to poke, but it's been 4 months so whats happening

arnodel

The only change I would request before merging is to update the README.md file which currently marks popen as unimplemented.

TorchedSammy · 2023-01-08T18:06:56Z

like that?

arnodel

It's going in

feat: implement io.popen

bae1f26

arnodel reviewed May 26, 2022

View reviewed changes

lib/iolib/iolib.go Show resolved Hide resolved

fix: handle errors and invalid mode

af63916

fix: file close on popen file

8482bad

TorchedSammy added 3 commits May 26, 2022 12:21

fix: io methods on popen file

a4a32c3

fix: file name on popen file

7ca2c4a

test: add io.popen tests

e623caa

TorchedSammy requested a review from arnodel May 26, 2022 16:53

TorchedSammy added 2 commits May 27, 2022 21:31

fix: add stdin and stdout to file struct

a0e2485

fix: use os pipe for dummy read/write when popen file opened in oppos…

3568273

…ite mode

TorchedSammy added 2 commits May 27, 2022 21:37

perf: use 1 pipe for in/out dummy instead of 2

cd03107

test: remove one of the popen tests that isnt needed

81f83af

arnodel reviewed May 28, 2022

View reviewed changes

lib/iolib/lua/iolib.lua Outdated Show resolved Hide resolved

arnodel reviewed May 28, 2022

View reviewed changes

lib/iolib/file.go Outdated Show resolved Hide resolved

TorchedSammy and others added 2 commits May 28, 2022 13:52

test: fix typo in io test

562b0c3

Co-authored-by: Arnaud Delobelle <arnodel@gmail.com>

refactor: use close callback and name in iolib file instead of cmd an…

68e1ee8

…d stdin/out

TorchedSammy requested a review from arnodel May 28, 2022 18:17

test: don't run popen tests on windows

8c75e6b

TorchedSammy added 3 commits June 2, 2022 19:11

test: revert "test: don't run popen tests on windows"

38b0e85

This reverts commit 8c75e6b.

chore: merge branch from upstream

d8fbfc1

test: use tags to separate windows and unix tests to remove failing w…

60061f9

…indows tests

arnodel reviewed Jun 3, 2022

View reviewed changes

chore: pull upstream changes

0593165

test(iolib): use backward slash for file path instead of forward slash

be2ba6a

TorchedSammy requested a review from arnodel August 19, 2022 13:05

arnodel requested changes Jan 8, 2023

View reviewed changes

docs: mark iolib as complete

cd624ec

TorchedSammy requested a review from arnodel January 8, 2023 18:06

arnodel approved these changes Jan 8, 2023

View reviewed changes

arnodel merged commit 0e5b284 into arnodel:lua5.4 Jan 8, 2023

feat: implement io.popen #88

feat: implement io.popen #88

Conversation

TorchedSammy commented May 25, 2022 • edited Loading

arnodel left a comment

Choose a reason for hiding this comment

codecov bot commented May 26, 2022 • edited Loading

Codecov Report

TorchedSammy commented May 26, 2022

arnodel commented May 26, 2022

TorchedSammy commented May 26, 2022

arnodel commented May 26, 2022

TorchedSammy commented May 26, 2022 • edited Loading

TorchedSammy commented May 26, 2022

TorchedSammy commented May 26, 2022 • edited Loading

TorchedSammy commented May 26, 2022

arnodel commented May 26, 2022 • edited Loading

TorchedSammy commented May 26, 2022

arnodel commented May 27, 2022

TorchedSammy commented May 28, 2022

TorchedSammy commented May 31, 2022

arnodel commented May 31, 2022

TorchedSammy commented May 31, 2022

TorchedSammy commented May 31, 2022

arnodel commented Jun 1, 2022

arnodel commented Jun 2, 2022 • edited Loading

TorchedSammy commented Jun 3, 2022

arnodel left a comment

Choose a reason for hiding this comment

arnodel Jun 3, 2022

Choose a reason for hiding this comment

TorchedSammy Jun 4, 2022

Choose a reason for hiding this comment

TorchedSammy commented Jun 4, 2022 • edited Loading

arnodel commented Jun 6, 2022

TorchedSammy commented Jun 6, 2022 • edited Loading

TorchedSammy commented Jun 21, 2022

arnodel commented Jun 22, 2022 via email

arnodel commented Jul 11, 2022

TorchedSammy commented Aug 17, 2022

TorchedSammy commented Aug 17, 2022 • edited Loading

TorchedSammy commented Dec 13, 2022

arnodel left a comment

Choose a reason for hiding this comment

TorchedSammy commented Jan 8, 2023

arnodel left a comment

Choose a reason for hiding this comment

TorchedSammy commented May 25, 2022 •

edited

Loading

codecov bot commented May 26, 2022 •

edited

Loading

TorchedSammy commented May 26, 2022 •

edited

Loading

TorchedSammy commented May 26, 2022 •

edited

Loading

arnodel commented May 26, 2022 •

edited

Loading

arnodel commented Jun 2, 2022 •

edited

Loading

TorchedSammy commented Jun 4, 2022 •

edited

Loading

TorchedSammy commented Jun 6, 2022 •

edited

Loading

TorchedSammy commented Aug 17, 2022 •

edited

Loading