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.

Sign up for GitHub

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

Jump to bottom

interp: move KillTimeout to DefaultExec #420

Closed
kaey opened this issue Sep 24, 2019 · 1 comment · Fixed by #434
Closed

interp: move KillTimeout to DefaultExec #420

kaey opened this issue Sep 24, 2019 · 1 comment · Fixed by #434

Comments

@kaey
Copy link
Contributor

kaey commented Sep 24, 2019 •
edited
Loading

KillTimeout is specific to DefaultExec, other exec implementations, like those written in go, don't benefit from it (you can't kill a goroutine).
I propose making DefaultExec a constructor, pass killTimeout as a param to it and remove it from interp.
func DefaultExec(killTimeout timeDuration) ExecModule {}
This also groups this function it under ExecModule type.
There is one more issue to make it more usable though, see #421
This was referenced Sep 24, 2019
Closed
Closed
@mvdan
Copy link
Owner

mvdan commented Sep 25, 2019

Hmm, sure. Other implementations could still benefit, such as a docker exec implementation, but the developer can set up the kill timeout themselves. PR welcome.

@kaey kaey mentioned this issue Oct 13, 2019
Merged
mvdan pushed a commit that referenced this issue Oct 13, 2019
@kaey @mvdan
interp: move KillTimeout to DefaultOpen
2ec8571 
KillTimeout is specific to DefaultExec,
other exec implementations, like those written in go,
don't benefit from it (you can't kill a goroutine).

Also updated DefaultOpen for consistency.
This also makes Default* functions group under their
respective type in godoc.

Fixes #420.
riacataquian added a commit to riacataquian/sh that referenced this issue Jul 3, 2022
@riacataquian
interp_test: fix sleep syntax compatibility
ef38d9f 
Running the tests on master gives me the following error:
```
--- FAIL: TestRunnerRunConfirm (0.03s)
    --- FAIL: TestRunnerRunConfirm/mvdan#420 (0.02s)
        interp_test.go:3690: wrong bash output in "f() { echo 1; }; { sleep 0.01s; f; } & f() { echo 2; }; wait":
            want: "1\n"
            got:  "usage: sleep seconds\n1\n"
```

I am running on `bash version 5.1.16(1)-release`:
```
$ bash --version
GNU bash, version 5.1.16(1)-release (aarch64-apple-darwin21.5.0)
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
```

The error I suppose, is because of the difference of the sleep
syntax on BSD/Unix/MacOS vs on GNU/Linux - the former requires
no unit and defaults to seconds, while the latter requires either
`smdh` as unit

To fix, fix the `sleep` test builtin to accept either of the syntax
and default to seconds when none is found

Finally, modify the tests to run without units so it would pass when
ran against real bash
riacataquian added a commit to riacataquian/sh that referenced this issue Jul 3, 2022
@riacataquian
interp_test: fix sleep syntax compatibility
c474e60 
Running the tests on master gives me the following error:
```
--- FAIL: TestRunnerRunConfirm (0.03s)
    --- FAIL: TestRunnerRunConfirm/mvdan#420 (0.02s)
        interp_test.go:3690: wrong bash output in "f() { echo 1; }; { sleep 0.01s; f; } & f() { echo 2; }; wait":
            want: "1\n"
            got:  "usage: sleep seconds\n1\n"
```

I am running on `bash version 5.1.16(1)-release`:
```
$ bash --version
GNU bash, version 5.1.16(1)-release (aarch64-apple-darwin21.5.0)
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
```

The error I suppose, is because of the difference of the sleep
syntax on BSD/Unix/MacOS vs on GNU/Linux - the former requires
no unit and defaults to seconds, while the latter requires either
`smdh` as unit

To fix, fix the `sleep` test builtin to accept either of the syntax
and default to seconds when none is found

Finally, modify the tests to run without units so it would pass when
ran against real bash
riacataquian added a commit to riacataquian/sh that referenced this issue Jul 3, 2022
@riacataquian
interp_test: fix sleep syntax compatibility
8748276 
Running the tests on master gives me the following error:
```
--- FAIL: TestRunnerRunConfirm (0.03s)
    --- FAIL: TestRunnerRunConfirm/mvdan#420 (0.02s)
        interp_test.go:3690: wrong bash output in "f() { echo 1; }; { sleep 0.01s; f; } & f() { echo 2; }; wait":
            want: "1\n"
            got:  "usage: sleep seconds\n1\n"
```

I am running on `bash version 5.1.16(1)-release`:
```
$ bash --version
GNU bash, version 5.1.16(1)-release (aarch64-apple-darwin21.5.0)
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
```

The error I suppose, is because of the difference of the sleep
syntax on BSD/Unix/MacOS vs on GNU/Linux - the former requires
no unit and defaults to seconds, while the latter requires either
`smdh` as unit

To fix, fix the `sleep` test builtin to accept either of the syntax
and default to seconds when none is found

Finally, modify the tests to run without units so it would pass when
ran against real bash
riacataquian added a commit to riacataquian/sh that referenced this issue Jul 4, 2022
@riacataquian
interp_test: fix sleep syntax compatibility
552b8f5 
Running the tests on master gives me the following error:
```
--- FAIL: TestRunnerRunConfirm (0.03s)
    --- FAIL: TestRunnerRunConfirm/mvdan#420 (0.02s)
        interp_test.go:3690: wrong bash output in "f() { echo 1; }; { sleep 0.01s; f; } & f() { echo 2; }; wait":
            want: "1\n"
            got:  "usage: sleep seconds\n1\n"
```

I am running on `bash version 5.1.16(1)-release`:
```
$ bash --version
GNU bash, version 5.1.16(1)-release (aarch64-apple-darwin21.5.0)
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
```

The error I suppose, is because of the difference of the sleep
syntax on BSD/Unix/MacOS vs on GNU/Linux - the former requires
no unit and defaults to seconds, while the latter requires either
`smdh` as unit

To fix, fix the `sleep` test builtin to accept either of the syntax
and default to seconds when none is found

Finally, modify the tests to run without units so it would pass when
ran against real bash
riacataquian added a commit that referenced this issue Jul 4, 2022
@riacataquian
interp_test: fix sleep syntax compatibility (#894)
8be48da 
Running the tests on master gives me the following error:
```
--- FAIL: TestRunnerRunConfirm (0.03s)
    --- FAIL: TestRunnerRunConfirm/#420 (0.02s)
        interp_test.go:3690: wrong bash output in "f() { echo 1; }; { sleep 0.01s; f; } & f() { echo 2; }; wait":
            want: "1\n"
            got:  "usage: sleep seconds\n1\n"
```

I am running on `bash version 5.1.16(1)-release`:
```
$ bash --version
GNU bash, version 5.1.16(1)-release (aarch64-apple-darwin21.5.0)
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
```

The error I suppose, is because of the difference of the sleep
syntax on BSD/Unix/MacOS vs on GNU/Linux - the former requires
no unit and defaults to seconds, while the latter requires either
`smdh` as unit

To fix, fix the `sleep` test builtin to accept either of the syntax
and default to seconds when none is found

Finally, modify the tests to run without units so it would pass when
ran against real bash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

interp: move KillTimeout to DefaultOpen
2 participants
@kaey @mvdan