Should FileSpec::Resolve() look at PATH?

I have an issue on Windows when trying to run shell commands. We specify the shell as “cmd.exe”, create a FileSpec out of this, and call FileSpec::Resolve. This ends up making an absolute path out of cmd.exe, but it does so by just sticking the working directory onto the front of it, which is obviously wrong.

My question is: For FileSpecs that are only filenames, nothing else, should we attempt to locate a matching file in PATH, and when we find one use the resulting absolute path?

Not by default. We should have a method on FileSpec that might say "ResolveExecutableUsingPath()", but I wouldn't do it by default for every file spec. Not every file is an executable, so we shouldn't treat all files as executables by default unless a method is explicitly called on that.

As a side questions, we provide a complete path to "/bin/sh" as the shell for unix, shouldn't we specify a full path to "cmd.exe" as well? Does it live in a known location on windows?

Greg

I was in the middle of typing the same thing pretty much as Greg... Delete that and just add a +1.

Jim

As a follow up, the Platform::ResolveExecutable() should be the one to resolve executables and the function call actually shouldn't go on FileSpec.

Although extremely uncommon, it’s theoretically possible to have a different system installation directory. Is this not possible on posix platforms? Is it guaranteed that /bin/sh is always the path, no matter what strange things a user might have done to their system?

Maybe a better solution is to have RunShellCommand() call Platform::ResolveExecutable.

Again, if you say:

(lldb) platform select remote-ios
(lldb) target create ls

I would expect it to find it in the iOS SDK, not in /bin/ls. So the currently selected platform is the one that must resolve this.

Although extremely uncommon, it's theoretically possible to have a different system installation directory. Is this not possible on posix platforms? Is it guaranteed that /bin/sh is always the path, no matter what strange things a user might have done to their system?

Yes this is possible. This is why you want the platform to resolve this. It can look at the environment for the current host platform if it is the host platform and "do the right thing".

Maybe a better solution is to have RunShellCommand() call Platform::ResolveExecutable.

That isn't required, maybe for windows? But if this the current platform is the host platform, then running a command like "sh foo.cmd" should find the "sh" in the current path.

Again, if you say:

(lldb) platform select remote-ios
(lldb) target create ls

I would expect it to find it in the iOS SDK, not in /bin/ls. So the
currently selected platform is the one that must resolve this.

Right but this is just target create, which is a different codepath than
Host::RunShellExecutable. I'm not clear on all the use cases of
Host::RunShellCommand aside from that it runs as part of the test suite,
but I can search for it all tomorrow. The point is that right
now, RunShellCommand is hard-coded to use /bin/ls. It doesn't resolve
anything using any platform.

I assume the codepath for handling "target create" already resolves the
executable, otherwise more basic stuff would have already broken.

>
> Although extremely uncommon, it's theoretically possible to have a
different system installation directory. Is this not possible on posix
platforms? Is it guaranteed that /bin/sh is always the path, no matter
what strange things a user might have done to their system?

Yes this is possible. This is why you want the platform to resolve this.
It can look at the environment for the current host platform if it is the
host platform and "do the right thing".

> Maybe a better solution is to have RunShellCommand() call
Platform::ResolveExecutable.

That isn't required, maybe for windows? But if this the current platform
is the host platform, then running a command like "sh foo.cmd" should find
the "sh" in the current path.

See first comment of this response. If it's possible that sh is not in
/bin, as you've said, then isn't it a bug that LLDB_DEFAULT_SHELL is
hardcoded to /bin/sh? If that's the case, then it seems to make sense to
change this to simply "sh", and then have Host::RunShellCommand call
Platform::ResolveExecutable for all platforms.

getenv(“COMSPEC”) rather than “cmd.exe”? I’ve never seen that be non-absolute.

It’s a little more difficult than that, because we have

#define LLDB_DEFAULT_SHELL “cmd.exe”

So we’d need to move this into a runtime function and fix up all assumptions about how it might be available at compile time. It doesn’t seem like that would be too bad though. The biggest offender would be that we currently have a function like this:

void Foo(int x, int y, const char *shell = LLDB_DEFAULT_SHELL)

But I suppose we could just add a two-argument overload that calls the 3 argument version with GetDefaultShell(), to avoid having to fix up a ton of callsites.

This might be a better solution than fixing it in RunShellCommand because it removes the possibility of someone forgetting to resolve the path in the future.

Thoughts Greg?

I would change the "shell" argument to a boolean "run_in_default_shell". I don't see any call sites of RunShellCommand that actually specify an alternate shell (though I may have missed one). Then if "run_in_default_shell" is true, we call GetDefaultShell().

Greg

I was a bit confused at how we could call a function named RunShellCommand with an option to not run it in a shell, but I went and looked over the code, and was surprised to find out this is actually a thing. Seems we pass NULL for the shell sometimes, with comments saying “Don’t run in shell”. Maybe someday I’ll try to change the name of this function to something less confusing, but for now I’ll implement it as you suggest.