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

suggest using exec foo for top-level commands rather than foo for consistency #163

Closed
mvdan opened this issue Jun 28, 2022 · 2 comments · Fixed by #164
Closed

suggest using exec foo for top-level commands rather than foo for consistency #163

mvdan opened this issue Jun 28, 2022 · 2 comments · Fixed by #164

Comments

@mvdan
Copy link
Collaborator

mvdan commented Jun 28, 2022

At https://github.com/cue-lang/cue we use test scripts for our integration tests, as well as for users providing reproducers for bugs.

There's one unfortunate inconsistency: the former use cue, and the latter use exec cue.

I initially thought that we should use cue everywhere, by adding a new flag to cmd/testscript like -x cue, so that a command "passes through" to executing a program from $PATH. Then, we would use cue everywhere rather than exec cue. However, then bug report reproducers aren't truly standalone, as one needs to remember to use the flag.

@rogpeppe suggests that we should use exec cue everywhere instead. This already works in today's testscript.RunMain, as top-level commands are put in a temporary directory and appended to $PATH. We simply prefer cue rather than exec cue since we're used to how it was before, where exec cue might fail to find the program if one hadn't run go install first, as we wouldn't fix up $PATH.

I think Roger's idea is better. It's slightly more verbose, but:

  1. It's consistent everywhere - tests and standalone reproducers.
  2. It keeps exec explicit, which is a good thing given that it's special. It uses stdin, sets stdout and stderr, runs as a separate process, can be run in the background, etc.
@mvdan
Copy link
Collaborator Author

mvdan commented Jun 28, 2022

Also: we should add a boolean flag like RequireExplicitExec so that users of RunMain can consistently use exec cue and not make the mistake of using cue inconsistently. RequireExplicitExec would make cue error, telling the user to use exec cue instead.

@myitcv
Copy link
Collaborator

myitcv commented Jun 28, 2022

You've won me over 😄

mvdan added a commit to mvdan/go-internal that referenced this issue Jun 28, 2022
We also document how top-level commands fed to RunMain work with and
without "exec" the same way, and how RequireExplicitExec can drop
backwards compatibility for greater consistency.

Fixes rogpeppe#163.
mvdan added a commit to mvdan/go-internal that referenced this issue Jun 28, 2022
We also document how top-level commands fed to RunMain work with and
without "exec" the same way, and how RequireExplicitExec can drop
backwards compatibility for greater consistency.

Fixes rogpeppe#163.
mvdan added a commit to mvdan/go-internal that referenced this issue Jul 6, 2022
We also document how top-level commands fed to RunMain work with and
without "exec" the same way, and how RequireExplicitExec can drop
backwards compatibility for greater consistency.

Fixes rogpeppe#163.
@mvdan mvdan closed this as completed in #164 Jul 6, 2022
mvdan added a commit that referenced this issue Jul 6, 2022
We also document how top-level commands fed to RunMain work with and
without "exec" the same way, and how RequireExplicitExec can drop
backwards compatibility for greater consistency.

Fixes #163.
porcuepine pushed a commit to cue-unity/unity that referenced this issue Aug 17, 2022
Use `exec git` and `exec unity` instead.
This removes the need for any setup at all,
but is also a better solution as `exec` can show stdout and stderr.
Moreover, we are moving testscript towards using `exec` consistently;
see rogpeppe/go-internal#163.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: I69edd03efc0f3cf5ca6c32f4c71783ebb9524ae4
Reviewed-on: https://review.gerrithub.io/c/cue-unity/unity/+/541992
TryBot-Result: <cue.porcuepine@gmail.com>
Reviewed-by: Paul Jolly <paul@myitcv.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants