- rG270e99ab0ae9: Fix some posix assumptions related to running shell commands.
rG00cdc98b7f58: Fix some posix assumptions related to running shell commands.
rLLDB223695: Fix some posix assumptions related to running shell commands.
rLLDB223548: Fix some posix assumptions related to running shell commands.
rL223695: Fix some posix assumptions related to running shell commands.
rL223548: Fix some posix assumptions related to running shell commands.
The only thing I don't know about is the:
Line... You might specify a tool on MacOSX that is i386 like "/path/to/i386/a.out" and if you set this to the system architecture x86_64-apple-macosx, it won't execute the shell tool. This should probably be removed. Did you need to do this for windows or did you just add be because you thought it would always be correct?
If you take a look at ConvertArgumentsForLaunchingInShell, I had to make a
decision about whether to use -c or /C as an argument to the shell. I
could have string checked the shell name against "cmd.exe", but that seemed
more hackish than asking "Is this going to run in a shell on windows"?
Also, there's some code in ConvertArgumentsForLaunchingInShell that checks
the architecture. If you get into that function through
Host::RunShellCommand, the architecture would have been uninitialized, so
it looked like in that case the code that was checking for it would have
The architecture check is in case you have a universal file that contains both x86_64 and i386, you might set the architecture to specify you want to run (debug more like it) the i386 slice even though you are on a 64 bit system (MacOSX supports this). For running a shell command, just let the system do what it needs to do and launch it using the best slice it can and don't set the architecture in the launch info.
So please remove "launch_info.SetArchitecture(HostInfo::GetArchitecture());" from Host::RunShellCommand().
In ConvertArgumentsForLaunchingInShell(), do you suggest I change the
conditional to use /C if the shell name is "cmd.exe"? Seems a little
hackish, so if there's a better way then that would be preferable.
What about setting the Architecture as I'm currently doing, but modify the
triple so it only contains OS and Environment, but not architecture?
Seems like we should either get this from a call like the one that gets "cmd.exe" (HostInfoWindows::GetDefaultShell()). In Host::RunShellCommand() we already do:
We should be asking the HostInfo class to be setting the shell arguments for us. Then we HostInfoPosix and HostInfoWindows could "do the right thing".
So I vote to remove ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell(), then create a function in the HostInfo class that can do this correctly for each system. This function is only used in Platform::LaunchProcess() when "IsHost()" is true and in Host::RunShellCommand().
Actually wait. Currently the architecture is completely uninitialized, so
anything that would break as a result of setting it in
Host::RunShellCommand would already break, because it would be making an
assumption that it could use an uninitialized ArchSpec. So this actually
seems a little better than before.
A more robust fix might involve breaking this function up into smaller
Looks like we crossed paths, but we basically said the same thing, that the function should be reworked. Can I commit the fix as-is (since it doesn't break anything, due to the existing Architecture being uninitialized), and then follow up with the re-work into HostInfo?
The reason for comitting as-is first is because this patch contains some other stuff, like the changes in FileSpec, and the temp file paths, that would be unrelated to a refactor of this function.
Ok, I will take out the checks I added against the triple in ConvertArgumentsForLaunchingInShell for now and commit, then re-add them once the logic is properly separated in HostInfo.
Just so I can understand the code better, can you point me to the code that would break if the Architecture were set? I see some in ConvertArgumentsForLaunchingInShell, but it's only exercised if will_debug == true, and RunShellCommand() uses will_debug == false. So I'm assuming its something deep inside of Host::LaunchProcess.
Yes, as checked in it didn't even build (fixed in r223559.) But then "run" fails with:
/bin/sh: exec /usr/bin/arch -arch x86_64 /Volumes/ThePlayground/Users/jingham/Projects/Sketch/build/Debug/Sketch.app/Contents/MacOS/Sketch: No such file or directory
error: process exited with status 127
But "process launch" with the same binary works correctly.
I resubmitted this in r223695. My original patch was actually ok, the
issue happened when I removed the code for setting the Architecture in the
ProcessLaunchInfo, I inadvertently removed the code that added -c to the
command line of /bin/sh. Since toward the end of the thread we determined
that setting the architecture was actually ok (since it's checked inside of
a will_debug block, I added this code back in.
I built and ran on both Linux and Mac, and did not see any failures. On
the off chance it does fail again though, let me know.