Remove hard dependency on bundled python_26 in tests

net/test/spawned_test_server/local_test_server_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/test/spawned_test_server/local_test_server.h"
 
 #include <windows.h>
 #include <wincrypt.h>
 
 #include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/environment.h"
 #include "base/files/file_path.h"
 #include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/process/launch.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/thread.h"
 #include "base/win/scoped_handle.h"
@@ skipping 55 matching lines @@
     return false;
   }
 
   return true;
 }
 
 }  // namespace
 
 namespace net {
 
+class AddedPythonPath {
+private:
+  std::string old_path_;
+  scoped_ptr<base::Environment> environment_;
+  bool path_modified_;
+
+  DISALLOW_COPY_AND_ASSIGN(AddedPythonPath);
+
+public:

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

nit: In Chromium style we usually put public: section first.

+  AddedPythonPath();
+  ~AddedPythonPath();
+};
+
+AddedPythonPath::AddedPythonPath()
+    : environment_(base::Environment::Create()),
+      path_modified_(false) {
+  // Retrieves the old path, adds third_party/python26 to the end of it and
+  // then restores the original path in the destructor.
+
+  (void)environment_->GetVar("PATH", &old_path_);

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

nit: Are these (void) casts needed? Generally we don't do C-style casts. If you
need to ignore a result of function call, use ignore_result from basictypes.h.

[Daniel Bratell, 2013/08/20 14:33:09]

Not needed. Just an old convention to silence static analyzers (including the
ones in compilers) and to show readers of the code that ignoring the result of
the function was intentional. I think it's helpful since readers might otherwise
think it was an oversight.

I'm switching to ignore_result (though I'm not sure static analyzers will handle
that as well), but I can also remove it completely if you prefer that.

[Paweł Hajdan Jr., 2013/08/22 18:11:53]

If these ignore_results are not needed to get it to compile, please remove them
(along with any comments about them).

+
+  std::string new_value = old_path_;
+  if (new_value.length() > 0)

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

nit: Why not do an .empty() check instead? The intent would be more obvious.

[Daniel Bratell, 2013/08/20 14:33:09]

Thanks! Just my string class confusion made me think there was no such method.
There are so many string classes (not just the ones in Chromium and STL. :-) )

+    new_value += ";"; // Path seperator.
+
+  // Add new path to the end so system pythons are used if available.

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

nit: system pythons -> system python (plural -> singular)

+  base::FilePath python_path;
+  if (!PathService::Get(base::DIR_SOURCE_ROOT, &python_path))

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

This is a silent failure, and the function that calls it returns bool.

Let's propagate the failure there.

[Daniel Bratell, 2013/08/20 14:33:09]

This is the constructor so you mean adding/using an exception? What exception do
you suggest?

I didn't consider this a fatal problem since I'm assuming every developer has a
python installed in a system path anyway but I might be wrong.

[Paweł Hajdan Jr., 2013/08/22 18:11:53]

We disable exceptions in Chrome. This means you'd need to avoid doing work in
constructor, and instead have a method with bool return value for this work and
WARN_UNUSED_RESULT.
This is an error with getting path to source root, nothing to do with presence
of python. :) It can fail in unexpected ways, especially under e.g. isolated
testing.

+    return;
+  python_path = python_path.Append(FILE_PATH_LITERAL("third_party"))

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

nit: You can just use AppendASCII.

+                           .Append(FILE_PATH_LITERAL("python_26"));
+  new_value += python_path.AsUTF8Unsafe();

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

Why the Unsafe method? :-/

Note that it may be needed to have an Environment that accepts native strings.

[Daniel Bratell, 2013/08/20 14:33:09]

The Environment class uses UTF-8 strings in the API. Apparently the "Unsafe"
part of the function name comes from it doing some magic stuff on some
non-Windows platforms and the function will be renamed as soon as that magic is
gone. Since this is pure Windows that doesn't apply here but yes, it looks a bit
bad. I will replace with WideToUTF8(python_path.value()).

[Paweł Hajdan Jr., 2013/08/22 18:11:53]

Sounds good!

+
+  path_modified_ = environment_->SetVar("PATH", new_value);
+}
+
+AddedPythonPath::~AddedPythonPath() {
+  if (path_modified_) {
+    if (old_path_.length() > 0)
+      (void)environment_->SetVar("PATH", old_path_);
+    else
+      (void)environment_->UnSetVar("PATH");
+  }
+}
+
 bool LocalTestServer::LaunchPython(const base::FilePath& testserver_path) {
   CommandLine python_command(CommandLine::NO_PROGRAM);
   if (!GetPythonCommand(&python_command))
     return false;
 
   python_command.AppendArgPath(testserver_path);
   if (!AddCommandLineArguments(&python_command))
     return false;
 
   HANDLE child_read = NULL;
@@ skipping 29 matching lines @@
   if (!job_handle_.IsValid()) {
     LOG(ERROR) << "Could not create JobObject.";
     return false;
   }
 
   if (!base::SetJobObjectAsKillOnJobClose(job_handle_.Get())) {
     LOG(ERROR) << "Could not SetInformationJobObject.";
     return false;
   }
 
+  AddedPythonPath python_path;
   base::LaunchOptions launch_options;
   launch_options.inherit_handles = true;
   launch_options.job_handle = job_handle_.Get();
   if (!base::LaunchProcess(python_command, launch_options, &process_handle_)) {
     LOG(ERROR) << "Failed to launch " << python_command.GetCommandLineString();
     return false;
   }
 
   return true;
 }
@@ skipping 19 matching lines @@
   if (!ParseServerData(server_data)) {
     LOG(ERROR) << "Could not parse server_data: " << server_data;
     return false;
   }
 
   return true;
 }
 
 }  // namespace net