Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21537002:
Remove hard dependency on bundled python_26 in tests (Closed)
|
Created:
7 years, 4 months ago by Daniel Bratell Modified:
7 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, msimonides Base URL:
https://chromium.googlesource.com/chromium/src.git@glyphcache_20130604 Visibility:
Public. |
DescriptionRemove hard dependency on bundled python_26 in tests
(solution suggested in https://codereview.chromium.org/16117004/ )
The hardcoded dependency on third_party/python_26 in some unit tests
makes it necessary to bundle that python with the test binaries. On other
platforms the system python is used and there is no reason Windows can't
do the same if one is available.
This saves us band-width (25 MB per test machine) when distributing tests.
The change is implemented as a removal of the hard coded path in execution and
instead it's inserted at the end of the PATH environment variable. That way it
will still be used if there is no other python available but otherwise a
system python will be used.
There is a small risk that the system python is unsuitable but that is a risk we
already take on non-Windows platforms.
Another solution was discussed in https://codereview.chromium.org/16117004/ before
this solution was suggested.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224096
Patch Set 1 #
Total comments: 6
Patch Set 2 : Using Chromium Types and reusing code. #
Total comments: 15
Patch Set 3 : Addressed some review issues. #
Total comments: 11
Patch Set 4 : Refactored more #
Total comments: 8
Patch Set 5 : Addressed review issues. #
Total comments: 2
Patch Set 6 : Better comments. #Messages
Total messages: 21 (0 generated)
This is a followup to https://codereview.chromium.org/16117004/ but since I'm not Marcin I had to open a new review. phajdan.jr suggested a solution like this there unless I misunderstood him.
Approach looks good to me. Some implementation comments. Thank you for the patch! https://codereview.chromium.org/21537002/diff/1/net/test/python_utils.cc File net/test/python_utils.cc (right): https://codereview.chromium.org/21537002/diff/1/net/test/python_utils.cc#newc... net/test/python_utils.cc:111: dir = base::FilePath(FILE_PATH_LITERAL("python.exe")); nit: Sort of a pre-existing thing, but "dir" is terrible name for this. Could you change it to e.g. python_exe or similar? Actually, would using just "python" work on Windows? If so, let's do that. https://codereview.chromium.org/21537002/diff/1/net/test/spawned_test_server/... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/1/net/test/spawned_test_server/... net/test/spawned_test_server/local_test_server_win.cc:98: AddedPythonPath::AddedPythonPath() : path_modified_(false), old_path_(NULL) { Could you reuse ScopedEnvironmentVariable from e.g. chrome/common/multi_process_lock_unittest.cc ? It can be moved to base or base/test (if you can't get it to base, I can accept it into base/test as the OWNER of it). https://codereview.chromium.org/21537002/diff/1/net/test/spawned_test_server/... net/test/spawned_test_server/local_test_server_win.cc:107: old_path_ = new TCHAR[result]; We generally avoid using TCHAR, TEXT, and LPTSTR directly unless really necessary. With base::Environment this should get slightly better.
Uploaded new patch (I will be away a few days so it's possible I'll not respond to followup questions until next week).
https://codereview.chromium.org/21537002/diff/1/net/test/python_utils.cc File net/test/python_utils.cc (right): https://codereview.chromium.org/21537002/diff/1/net/test/python_utils.cc#newc... net/test/python_utils.cc:111: dir = base::FilePath(FILE_PATH_LITERAL("python.exe")); On 2013/08/01 17:56:28, Paweł Hajdan Jr. wrote: > nit: Sort of a pre-existing thing, but "dir" is terrible name for this. Could > you change it to e.g. python_exe or similar? > > Actually, would using just "python" work on Windows? If so, let's do that. It means that we'll start using python.bat from depot_tools but it seems to work so why not. Did that change. Fewer lines of code. :-) https://codereview.chromium.org/21537002/diff/1/net/test/spawned_test_server/... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/1/net/test/spawned_test_server/... net/test/spawned_test_server/local_test_server_win.cc:98: AddedPythonPath::AddedPythonPath() : path_modified_(false), old_path_(NULL) { On 2013/08/01 17:56:28, Paweł Hajdan Jr. wrote: > Could you reuse ScopedEnvironmentVariable from e.g. > chrome/common/multi_process_lock_unittest.cc ? It can be moved to base or > base/test (if you can't get it to base, I can accept it into base/test as the > OWNER of it). I took a look at it and it has slightly different semantics since it will unconditionally remove the variable after using it. It's also not very big so there wouldn't be much code used from it. But I did take the hint from its use of base::Environment which means that this class also shrank a lot. Thanks!
phajdan.jr, can you have a look at the updated patch please?
https://codereview.chromium.org/21537002/diff/1/net/test/spawned_test_server/... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/1/net/test/spawned_test_server/... net/test/spawned_test_server/local_test_server_win.cc:98: AddedPythonPath::AddedPythonPath() : path_modified_(false), old_path_(NULL) { On 2013/08/02 07:27:53, bratell wrote: > On 2013/08/01 17:56:28, Paweł Hajdan Jr. wrote: > > Could you reuse ScopedEnvironmentVariable from e.g. > > chrome/common/multi_process_lock_unittest.cc ? It can be moved to base or > > base/test (if you can't get it to base, I can accept it into base/test as the > > OWNER of it). > > I took a look at it and it has slightly different semantics since it will > unconditionally remove the variable after using it. It's also not very big so > there wouldn't be much code used from it. But I did take the hint from its use > of base::Environment which means that this class also shrank a lot. Thanks! Let's make the version you created for this purpose reusable and re-use it in multi_process_lock_unittest.cc . https://codereview.chromium.org/21537002/diff/7001/net/test/python_utils.cc File net/test/python_utils.cc (right): https://codereview.chromium.org/21537002/diff/7001/net/test/python_utils.cc#n... net/test/python_utils.cc:112: python_cmd->SetProgram(dir); nit: If it fits, just put FILE_PATH_LITERAL("python") right on this line, no need for a variable. Otherwise, rename it to e.g. python_exe or python_bin or similar. https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:97: public: nit: In Chromium style we usually put public: section first. https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:108: (void)environment_->GetVar("PATH", &old_path_); 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. https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:111: if (new_value.length() > 0) nit: Why not do an .empty() check instead? The intent would be more obvious. https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:114: // Add new path to the end so system pythons are used if available. nit: system pythons -> system python (plural -> singular) https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:116: if (!PathService::Get(base::DIR_SOURCE_ROOT, &python_path)) This is a silent failure, and the function that calls it returns bool. Let's propagate the failure there. https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:118: python_path = python_path.Append(FILE_PATH_LITERAL("third_party")) nit: You can just use AppendASCII. https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:120: new_value += python_path.AsUTF8Unsafe(); Why the Unsafe method? :-/ Note that it may be needed to have an Environment that accepts native strings.
Everything addressed (I think) except the "use this for other tests as well" which I'm going to do next. https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:108: (void)environment_->GetVar("PATH", &old_path_); On 2013/08/19 19:16:56, Paweł Hajdan Jr. wrote: > 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. 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. https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:111: if (new_value.length() > 0) On 2013/08/19 19:16:56, Paweł Hajdan Jr. wrote: > nit: Why not do an .empty() check instead? The intent would be more obvious. 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. :-) ) https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:116: if (!PathService::Get(base::DIR_SOURCE_ROOT, &python_path)) On 2013/08/19 19:16:56, Paweł Hajdan Jr. wrote: > This is a silent failure, and the function that calls it returns bool. > > Let's propagate the failure there. 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. https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:120: new_value += python_path.AsUTF8Unsafe(); On 2013/08/19 19:16:56, Paweł Hajdan Jr. wrote: > Why the Unsafe method? :-/ > > Note that it may be needed to have an Environment that accepts native strings. 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()).
On 2013/08/19 19:16:55, Paweł Hajdan Jr. wrote: > Let's make the version you created for this purpose reusable and re-use it in > multi_process_lock_unittest.cc . multi_process_lock_unittest.cc is similar but still quite different. ScopedEnvironmentVariable is about temporarily setting and then removing an environment variable. AddedPythonPath is about temporarily modifying a variable according to the rules for PATH variables (on Windows). A class that can do both will not be very different from the two classes each by itself, just slightly more complex, so I think the reusable code is at the right level already (the Environment class).
On 2013/08/21 13:39:17, bratell wrote: > On 2013/08/19 19:16:55, Paweł Hajdan Jr. wrote: > > Let's make the version you created for this purpose reusable and re-use it in > > multi_process_lock_unittest.cc . > > multi_process_lock_unittest.cc is similar but still quite different. phajdan.jr, can I commit this without that change at first (after the last patch has been reviewed and potentially changed)? I've done it locally on a branch and I can put that up on a separate review as soon as this has landed but since I'm not convinced it's an overall improvement I'd rather not mess up this review with it.
Thanks for the patch, sorry about small delays. Lots of tiny comments, should be ready after that (I'd like to take another look though). https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:108: (void)environment_->GetVar("PATH", &old_path_); On 2013/08/20 14:33:09, bratell wrote: > On 2013/08/19 19:16:56, Paweł Hajdan Jr. wrote: > > 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. > > 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. If these ignore_results are not needed to get it to compile, please remove them (along with any comments about them). https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:116: if (!PathService::Get(base::DIR_SOURCE_ROOT, &python_path)) On 2013/08/20 14:33:09, bratell wrote: > On 2013/08/19 19:16:56, Paweł Hajdan Jr. wrote: > > This is a silent failure, and the function that calls it returns bool. > > > > Let's propagate the failure there. > > This is the constructor so you mean adding/using an exception? What exception do > you suggest? 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. > 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. 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. https://codereview.chromium.org/21537002/diff/7001/net/test/spawned_test_serv... net/test/spawned_test_server/local_test_server_win.cc:120: new_value += python_path.AsUTF8Unsafe(); On 2013/08/20 14:33:09, bratell wrote: > I will replace with WideToUTF8(python_path.value()). Sounds good! https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:89: class AddedPythonPath { nit: Rename to ScopedPath, and move to anonymous namespace. Add a TODO to share this code more broadly. https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:90: public: nit: Indent +1. https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:94: private: nit: Indent +1. https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:95: std::string old_path_; nit: Please comment all the members. https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:112: new_value += ";"; // Path seperator. nit: No need for the comment (by the way, we'd use two spaces between comment and code per the style guide). https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:116: if (!PathService::Get(base::DIR_SOURCE_ROOT, &python_path)) Please propagate this error as said before... https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:118: python_path = python_path.AppendASCII("third_party") Actually it'd be better to pass this as a parameter to the ctor. https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:119: .AppendASCII("python_26"); nit: This is a bit weird wrapping - if it doesn't fit entirely on the previous line, could you rather wrap after "=" or "("?
New version. It has grown some due to the generalizations so it's less compact but otherwise it does the same. https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:89: class AddedPythonPath { On 2013/08/22 18:11:53, Paweł Hajdan Jr. wrote: > nit: Rename to ScopedPath, and move to anonymous namespace. > > Add a TODO to share this code more broadly. Won't ScopedPath feel like a strange name for something that specifically adds a python distribution to the path. Maybe ScopedPythonPath? https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:119: .AppendASCII("python_26"); On 2013/08/22 18:11:53, Paweł Hajdan Jr. wrote: > nit: This is a bit weird wrapping - if it doesn't fit entirely on the previous > line, could you rather wrap after "=" or "("? It was the way the code looked before so I just assumed it was the preferred way in this code. Takes a while to learn every owners favourite style so I don't assume that anything is wrong just because it looks strange.
https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/15001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:89: class AddedPythonPath { On 2013/08/23 17:42:30, bratell wrote: > On 2013/08/22 18:11:53, Paweł Hajdan Jr. wrote: > > nit: Rename to ScopedPath, and move to anonymous namespace. > > > > Add a TODO to share this code more broadly. > > Won't ScopedPath feel like a strange name for something that specifically adds a > python distribution to the path. Maybe ScopedPythonPath? Ignore this. I wrote this comment before reading your other comments and forgot to delete it.
Just minor comments. https://codereview.chromium.org/21537002/diff/22001/net/test/spawned_test_ser... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/22001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:88: // TODO: By making this more generic we can possibly reused it at nit: TODO should have a username in parentheses. Ideally TODO(bratell), but feel free to say TODO(phajdan.jr). See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C... https://codereview.chromium.org/21537002/diff/22001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:92: // third_party/python_26. nit: Don't mention python_26 here as it's supposed to be generic. https://codereview.chromium.org/21537002/diff/22001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:93: ScopedPath(const base::FilePath& path_to_add); nit: One-parameter ctros should be marked explicit. https://codereview.chromium.org/21537002/diff/22001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:109: // haven't then we should also not try to "restore" it in the nit: No need for the "If we haven't" part (up to you though - feel free to keep if you think it should be there). https://codereview.chromium.org/21537002/diff/22001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:113: // This is a local stack based helper object. Copying it would make nit: DISALLOW_COPY_AND_ASSIGN is a common enough macro that it doesn't need a comment. https://codereview.chromium.org/21537002/diff/22001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:121: // Retrieves the old PATH, adds the new path to the end of it and nit: No need for this comment. https://codereview.chromium.org/21537002/diff/22001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:135: if (path_modified_) { nit: I prefer an early return, like: if (!path_modified_) return; if (old_path_.empty()) ... https://codereview.chromium.org/21537002/diff/22001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:147: nit: No need for this empty line.
phajdan.jr, I think the version I downloaded last has addressed your issues. Could you please take a look?
On 2013/09/05 11:17:00, Daniel Bratell wrote: > phajdan.jr, I think the version I downloaded last has addressed your issues. > Could you please take a look? uploaded!
LGTM with nits https://codereview.chromium.org/21537002/diff/28001/net/test/spawned_test_ser... File net/test/spawned_test_server/local_test_server_win.cc (right): https://codereview.chromium.org/21537002/diff/28001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:88: // TODO(bratell): By making this more generic we can possibly reused nit: TODO in this place is surprising. Please put it above "class ScopedPath" instead and fix indentation. Sorry I didn't notice it in the previous version. https://codereview.chromium.org/21537002/diff/28001/net/test/spawned_test_ser... net/test/spawned_test_server/local_test_server_win.cc:92: // Constructor which sets up the environment to include the path to nit: "to include the path to path_to_add" -> "to add |path_to_add| to PATH"
On 2013/09/05 18:31:58, Paweł Hajdan Jr. wrote: > LGTM with nits > Fixed I think. My biggest worry is still that it will cause problems on some test server somewhere but I don't think it's an unreasonable change.
On 2013/09/05 20:28:11, Daniel Bratell wrote: > On 2013/09/05 18:31:58, Paweł Hajdan Jr. wrote: > > LGTM with nits > > > > Fixed I think. > > My biggest worry is still that it will cause problems on some test server > somewhere but I don't think it's an unreasonable change. phajdan.jr, can you ok the last fix? Or is it enough that the second to last patch is oked?
LGTM At least when I use it "LGTM with ..." means you're good to go after applying the comments.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bratell@opera.com/21537002/34001
Message was sent while issue was closed.
Change committed as 224096 |
