[PATCH] D48050: [lit] Split test_set_working_dir TestProcessLaunch into two tests and fix it on Windows

[Pavel Labath via Phabricator via llvm-commits]

labath accepted this revision.
labath added a reviewer: jingham.
labath added a comment.
This revision is now accepted and ready to land.

The change looks good. The only potentially risky thing is the maxLaunchCount change, but the fact that it is overridable by an env var should mitigate that. It looks like a very weird hack that seems to have been here since 2010. I hope we can remove it altogether at some point, and the requirement for configurations (if any) which require it to explicitly request that behavior seems like good step towards that.



================
Comment at: source/Plugins/Process/Windows/Common/ProcessWindows.cpp:237-246
+
+  FileSpec working_dir = launch_info.GetWorkingDirectory();
+  namespace fs = llvm::sys::fs;
+  if (working_dir && (!working_dir.ResolvePath() ||
+                      !fs::is_directory(working_dir.GetPath()))) {
+    result.SetErrorStringWithFormat("No such file or directory: %s",
+                                   working_dir.GetCString());
----------------
Checking the directory this way makes the launching subject to race conditions (cwd removed between this check and the actual launch). It's not very important because the launch will presumably still fail, just probably with a less helpful error message. However, if there is a way to detect this scenario from the CreateProcess error code, it would be better to do it that way.


Repository:
  rL LLVM

https://reviews.llvm.org/D48050