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

Better environment control when spawning processes (develop) #832

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

kunitoki
Copy link

Add support for better environment variables in the framework, because since today it was not possible to start a process and control which environment should have:

  • Added functions to SystemStats to allow getting and changing the environment:
bool setEnvironmentVariable(const String& name, const String& value);

bool removeEnvironmentVariable(const String& name);

StringPairArray getEnvironmentVariables();
  • Added functions to ChildProcess to pass the environment variables the child process should retain:
bool start (const String& command, const StringPairArray& environment, int streamFlags = wantStdOut | wantStdErr)
bool start (const StringArray& arguments, const StringPairArray& environment, int streamFlags = wantStdOut | wantStdErr)
  • Added a function to Process to pass the environment variables the process spawned should retain:
static bool JUCE_CALLTYPE openDocument (
    const String& documentURL,
    const String& parameters,
    const StringPairArray& environment)
  • Fixed macOS Process::openDocument to correctly spawn .app bundles with arguments and environment, and well as urls and local files.

@kunitoki kunitoki changed the title Better environment control when spawning processes Better environment control when spawning processes (develop) Jan 15, 2021
@@ -445,15 +482,15 @@ class ChildProcess::ActiveProcess

if (available == 0)
{
if (! isRunning())
if (total != 0 || ! isRunning())
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to produce the output faster on windows and avoid spinning if we alread have characters. this will make the shell output flow more fluently instead of getting out very slow and clunky on processes that produce lot of output (like build systems).

Thread::sleep (1);
}
else
{
DWORD numRead = 0;
if (! ReadFile ((HANDLE) readPipe, dest, (DWORD) numToDo, &numRead, nullptr))
if (! ReadFile ((HANDLE) readPipe, dest, (DWORD) numToDo, &numRead, nullptr) || numRead == 0)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also this one

Comment on lines +956 to +970
auto oldEnvironment = SystemStats::getEnvironmentVariables();
for (const auto& key : oldEnvironment.getAllKeys())
SystemStats::removeEnvironmentVariable (key);

for (const auto& key : environment.getAllKeys())
SystemStats::setEnvironmentVariable (key, environment.getValue (key, {}));

HINSTANCE hInstance = ShellExecute (0, 0, fileName.toWideCharPointer(),
parameters.toWideCharPointer(), 0, SW_SHOWDEFAULT);

for (const auto& key : environment.getAllKeys())
SystemStats::removeEnvironmentVariable (key);

for (const auto& key : oldEnvironment.getAllKeys())
SystemStats::setEnvironmentVariable (key, oldEnvironment.getValue (key, {}));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unluckily ShellExecute inherit the environment and it doesn't allow to specify a custom environment like execve, so i had to modify the env in place

#endif

if (readHandle == nullptr && childPID != 0)
readHandle = fdopen (pipeHandle, "r");
Copy link
Author

@kunitoki kunitoki Jan 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by not doing this we can use use non buffering and non blocking read instead of fread which can block the process if no output is produced. speed is improved and you won't get character out in chunks with lot of latency and wait time between buffer are filled with new data. it is especially noticeable if you trigger cmake with lots of output from a juce ChildProcess.

@kunitoki
Copy link
Author

@tpoole thanks for pointing out the PR i made on juce6 has been closed automatically. Any chance these changes will be considered ?

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 this pull request may close these issues.

1 participant