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

Way to execute from a different directory? #112

Open
ghostsquad opened this issue Apr 20, 2022 · 44 comments
Open

Way to execute from a different directory? #112

ghostsquad opened this issue Apr 20, 2022 · 44 comments

Comments

@ghostsquad
Copy link

How would you choose the directory the script.Exec is running in?

@bitfield
Copy link
Owner

I think os.Chdir should do this, shouldn't it?

@ghostsquad
Copy link
Author

Well I'm looking to execute from a different directory without actually changing my current directory, especially as os.chdir is going cause problems if I do any sort of parallelism/threading.

@bitfield
Copy link
Owner

Yes, I see your point. The exec.Cmd does have a Dir field to control this, but currently script doesn't provide any way to set it for the command executed by Exec. I'm not sure what a good API would look like—any ideas?

@schrej
Copy link

schrej commented Apr 26, 2022

A backwards compatible approach would be using variadic arguments to allow passing in optional parameters. This is a pretty common approach and is often used with structs or functions. Unfortunately I don't know if there is a name for that pattern.

I prefer the approach using functions.

type ExecOption func(&exec.Cmd)

func ExecWithDir(dir string) ExecOption {
    return func(c &exec.Cmd) {
        c.Dir = dir
    }
}

func (p *Pipe) Exec(command string, opts ...ExecOption) *Pipe {
	return p.Filter(func(r io.Reader, w io.Writer) error {
		args, ok := shell.Split(command) // strings.Fields doesn't handle quotes
		if !ok {
			return fmt.Errorf("unbalanced quotes or backslashes in [%s]", command)
		}
		cmd := exec.Command(args[0], args[1:]...)
		cmd.Stdin = r
		cmd.Stdout = w
		cmd.Stderr = w
                for _, o := range opts {
                  o(cmd)
                }
		err := cmd.Start()
		if err != nil {
			fmt.Fprintln(w, err)
			return err
		}
		return cmd.Wait()
	})
}

func main() {
      Exec("ls", ExecWithDir("/root")).Stdout()
}

@ghostsquad
Copy link
Author

ghostsquad commented Apr 26, 2022

Your example is the functional options API pattern. It's what I use too. It's very good.

@schrej
Copy link

schrej commented Apr 26, 2022

@ghostsquad TIL, thank you!

The Blogpost about the talk that might have given it that name: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

@ghostsquad
Copy link
Author

That's the one! A really fantastic post. I love the flexibility it provides, and enables you to add functionality to an API without changing the function signatures.

@bitfield
Copy link
Owner

Functional options is definitely a possibility here, though it ends up being a little awkward because the option names need to be prefixed with the package name. A more realistic version of the example given is:

script.Exec("/bin/ls", script.ExecWithDir("/some/directory/path")).Stdout()

...which tends to obscure the otherwise fluent pipeline syntax. You can imagine that if there were more options, this would soon no longer be a one-liner.

@schrej
Copy link

schrej commented Apr 27, 2022

That's true. But if you have complex pipelines like that, you could also write them in multiple lines, e.g.

script.Exec("/bin/ls", script.ExecWithDir("/some/directory/path")).
  Match("something").
  Stdout()

which is still very readable.

@bitfield
Copy link
Owner

The flow-breaking is still there, even if you spread it across multiple lines. And there's a question about what happens if you pipe the output of this Exec into some other Exec, for example: what working directory does that one operate with?

I think if we were going to set the working directory for a pipe, which I must say I'm not yet convinced is necessary, I think it would be more useful to have this be an option for the pipe as a whole:

script.NewPipe().WithWorkingDirectory("/home/me").Exec(...)

@schrej
Copy link

schrej commented Apr 27, 2022

If other methods like File() support having a working directory as well, that would also be a good API. I would truncate it to something like WithWorkDir() though, otherwise it makes those one-liners pretty long.

@bitfield
Copy link
Owner

Or even just:

script.NewPipe().At("/home/me")...

@thiagonache
Copy link
Contributor

I'm trying to figure out why we can't just run

os.Chdir("home/me")
script.Exec("ls /tmp").Stdout()

@ghostsquad
Copy link
Author

Because os.chdir is global. Not thread safe. It's not specific to the command.

@thiagonache
Copy link
Contributor

You paid too much attention to the os.chdir. It was just to show that no matter what’s the path you are you can pass via args to the cmd itself instead of setting it on the cwd field of exec without changing anything on the package

@ghostsquad
Copy link
Author

You paid too much attention to the os.chdir. It was just to show that no matter what’s the path you are you can pass via args to the cmd itself instead of setting it on the cwd field of exec without changing anything on the package

I'm confused by this statement. You just said:

I'm trying to figure out why we can't just run

os.Chdir("home/me")
script.Exec("ls /tmp").Stdout()

My answer is because os.Chdir is not specific to script.Exec. Are you suggesting that I run something like this?

workdir := "/foo/bar")
cmd := "ls"
script.Exec(fmt.Sprintf("cd %q && cmd", workdir, cmd)).Stdout()

though I agree this is pretty common in shell scripts, e.g.

(
    cd "${workdir}";
    ls
)

It just feels really hacky, and there's not a whole lot of room for script to do any sort of validation before hand, such as checking to see if the workdir actually exists yet. It also forces script to treat the command as a shell script, and not a binary, because of the &&. It's not the same.

@thiagonache
Copy link
Contributor

thiagonache commented May 16, 2022

my idea is even simpler

workdir := "/foo/bar"
cmd := "ls"
script.Exec(fmt.Sprintf("cmd workdir", workdir, cmd)).Stdout()

@ghostsquad
Copy link
Author

What to do in the case that the command doesn't accept a path. ls is an over simplified example. Better example may be make.

@bitfield
Copy link
Owner

bitfield commented Jun 2, 2022

It's a good suggestion, but I don't think I've seen enough widespread support to convince me that the lack of such a mechanism is a serious problem for a large number of users. Let's close for now and return to this if the issue is raised again by others.

@ghostsquad
Copy link
Author

ghostsquad commented Aug 14, 2022

Just coming back to this, and I realized this has been asked for in various ways in several other issues, all of which dismissed in some form.

#79, #109, #48, #8

Just to reiterate, using os.Chdir is not going to work, as it is applies "globally", and is subject to race conditions when executing multiple external binaries or commands in parallel.

I'm just looking for something equivalent to setting the Path field in os/exec#Cmd.

@bitfield
Copy link
Owner

By all means make a specific proposal!

What about this, for example:

script.At("/home/john").Exec("ls")

@ghostsquad
Copy link
Author

By all means make a specific proposal!

What about this, for example:

script.At("/home/john").Exec("ls")

I do like that.

@bitfield
Copy link
Owner

Let's see if we can come up with one or two more realistic examples of programs using At, to get a better idea of whether this API works.

@ghostsquad
Copy link
Author

(
  cd "./project"
  make
)

@bitfield bitfield reopened this Aug 17, 2022
@bitfield
Copy link
Owner

What would really help us here is a program that:

  1. Solves some real user problem (so that it's worth writing the program at all)
  2. Uses the proposed API (script.At)
  3. Executes multiple commands in parallel with different working directories (otherwise we don't need At, because os.Chdir will be fine)
  4. Is better written with script than the standard library, or some other package

@ghostsquad
Copy link
Author

https://gobyexample.com/goroutines

Replace fmt.Println with running a shell command.

@ghostsquad
Copy link
Author

ghostsquad commented Aug 17, 2022

I'm not writing a program here to prove this feature is useful. It seems you don't believe that this feature is useful. I'll just close this and I'll go somewhere else.

Thank you for your time.

@ghostsquad ghostsquad closed this as not planned Won't fix, can't repro, duplicate, stale Aug 17, 2022
@bitfield
Copy link
Owner

bitfield commented Aug 17, 2022

It's not a matter of belief: we simply don't need features that aren't needed!

In other words, software exists not to provide employment for software engineers, but to solve user problems. If no one has a problem that this feature solves, it's better not to write it. Useless API methods serve only to clutter the program and make it harder to use.

You have several times advanced the idea that there is a real use case requiring this feature, but despite many invitations, you've refused to say what it is. If you or anyone else can provide one, we'll implement this feature like a shot!

@ghostsquad
Copy link
Author

If no one has a problem that this feature solves, it's better not to write it.

I agree that features that no one will use should not exist, it's a waste of time. I have not yet met someone who openly admits to posting issues on GitHub for shits and giggles.

You have several times advanced the idea that there is a real use case requiring this feature, but despite many invitations, you've refused to say what it is.

4 months ago I posted this:

Well I'm looking to execute from a different directory without actually changing my current directory, especially as os.chdir is going cause problems if I do any sort of parallelism/threading.

I have a use case for this feature.

Please let me know if I've been unclear in any way.

@bitfield
Copy link
Owner

Yes, I think I see where the confusion is coming from. "Use case" means different things to different people, but in this case I mean a real-life problem that that can only be solved if this feature is present.

For example, maybe you want to build several projects simultaneously, so you need to concurrently run make in a number of separate directories. You'd like to write something like:

for _, p := range projects {
	go script.At(p).Exec("make").Stdout()
}

That's the kind of program I was asking for. Sorry that I was unclear—I don't know the right words to use to ask people to supply these examples.

@ghostsquad
Copy link
Author

ghostsquad commented Aug 17, 2022

Is there a way I could have reworded this statement to improve clarity?

especially as os.chdir is going cause problems if I do any sort of parallelism/threading.

@ghostsquad ghostsquad reopened this Aug 17, 2022
@bitfield
Copy link
Owner

Yes, I think so. What you've given there is a solution, not a problem. In other words, you're framing your request in terms of "I want Feature X". That's totally legit, but it doesn't help the maintainers understand why you're asking for Feature X.

The reason for asking why is not gatekeeping, or demanding that you justify the need for Feature X. It's to help us understand what the underlying problem is that you're trying to solve, to which you think (I'm sure correctly) that Feature X is the solution.

And the reason we need to understand that is so that we can design the feature in such a way that it's best suited to the real problem you have, not some imaginary problem that we might come up with. What would a description of that problem look like? Something like the example I gave: "I want to build several projects simultaneously, by concurrently running make in a number of different directories."

That may not be the problem you have (I'm exercising my imagination, which is what we're saying maintainers shouldn't do), but it is at least a possible answer to the question "What kind of real-world problem would Feature X solve?"

@ghostsquad
Copy link
Author

ghostsquad commented Aug 18, 2022

That wasn't a solution at all. Someone else proposed os.chdir, which I said would not work because it's not threadsafe.

Thus, a threadsafe mechanism to manage the current working directory is in fact the problem that I want to solve.

@ghostsquad
Copy link
Author

I mention threadsafety a second time here:

Because os.chdir is global. Not thread safe. It's not specific to the command.

And a third time in this comment:

Just to reiterate, using os.Chdir is not going to work, as it is applies "globally", and is subject to race conditions when executing multiple external binaries or commands in parallel.

I'm just looking for something equivalent to setting the Path field in os/exec#Cmd.

@thiagonache
Copy link
Contributor

thiagonache commented Oct 11, 2022 via email

@wburningham
Copy link

I have a real-world example where I need to create a temporary directory, git clone a repo into that directory, then do some "work" (the work could be updating project dependencies, running static analysis, executing code modification, etc).

I can create the temporary directory before invoking github.com/bitfield/script (nice-to-have if that was another part of the API), but then I need to be able to pass the path to At (or whatever name you come up with).

Is that helpful?

@wburningham
Copy link

wburningham commented Nov 30, 2022

I guess in my case I can just use something like: script.Exec("git clone <repo< <tmp dir>").Stdout()

But then certain tools executed via script.Exec must be executed from within the directory (ex: the go tool).

@ghostsquad
Copy link
Author

Thanks @wburningham for pointing out that go is one of those tools that cares about the working directory. There are a 1000 others, that I should not need to mention in order to make a request that I can change the working directory in a threadsafe way.

What you've given there is a solution, not a problem.

Here's the problem: I cannot execute a command while controlling the working directory in a threadsafe way.

@mwittie
Copy link

mwittie commented May 19, 2023

FWIW, I like the script.At("location").Exec("cmd") syntax and would find it useful. Right now I need to use other go packages to run bash -c ls location && cmd, which is not particularly syntax agnostic.

@partiallyordered
Copy link

partiallyordered commented Sep 26, 2023

Piling on here a little: git-crypt is another tool which I'd like to call using script.Exec, but doesn't support a target directory parameter. Perhaps ideally all tools would support a target directory parameter (but perhaps not!) but we can't expect the whole world to change for us.

In general, I'd like to

  • be able to execute a script pipeline without worrying that something else (in my code, or a dependency) will change the working directory in the background
  • not clobber the working directory (by calling os.Chdir) for the rest of my code, or a consumer of my package
  • not have to manually manage the working directory when I execute a script pipeline (set os.Chdir, then revert it afterward)

I'm a pretty new script user and a bit green with Go so I'm not really sure how this would most ergonomically/maintainably be implemented, so I won't propose an API- sorry that's not much use. I will say that the .At proposal above looks like it could support not only .Exec but potentially .ListFiles and .FindFiles. This could be more confusing than useful though, and I'm not sure I can see a use-case where setting the working directory per-pipeline would be so much better than setting it per-method.

To the maintainer: thanks, script has been great so far.

@krolikbrunatny
Copy link

I use this package heavily in my automation script(s). There are cases where this feature would be useful. For example, I pull a few repositories in parallel and execute commands in the cloned directories. While I can use exec.Command(...) as a workaround, I'd love to keep my code consistent with script.Exec(...).

@mahadzaryab1
Copy link
Contributor

@bitfield Looks like there quite a few requests for this feature. Can I pick it up?

@bitfield
Copy link
Owner

bitfield commented Sep 8, 2024

Certainly, and here's where we've been stuck up to now:

What would really help us here is a program that:

  1. Solves some real user problem (so that it's worth writing the program at all)
  2. Uses the proposed API (script.At)
  3. Executes multiple commands in parallel with different working directories (otherwise we don't need At, because os.Chdir will be fine)
  4. Is better written with script than the standard library, or some other package

@mahadzaryab1
Copy link
Contributor

@krolikbrunatny I'm trying to put together a proposal for this feature. You mentioned that you use this package from an automation script. Would you be able to show me an example of how you're working around not having this feature with exec.Command? Alternatively, it would help to see how you would use the functionality if it was included in script.

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

No branches or pull requests

9 participants