Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2420543002:
Use passed-in --build-dir instead of GetBuildOutputDirectory in archive_layout_test_results.py. (Closed)
DescriptionUse passed-in --build-dir instead of GetBuildOutputDirectory in archive_layout_test_results.py.
BUG=655798
Committed: https://chromium.googlesource.com/chromium/tools/build/+/072fdd58f2070edbb909079ff8523ce6a38c8d65
Patch Set 1 #Patch Set 2 : Don't pass unused parameter --build-dir to archive_layout_test_results.py. #Patch Set 3 : Use the passed-in build-dir #
Total comments: 5
Patch Set 4 : build_dir -> options.build_dir, revert changes in steps.py #Messages
Total messages: 17 (4 generated)
qyearsley@chromium.org changed reviewers: + dpranke@chromium.org, pfeldman@chromium.org
This isn't actually unused, right? Rather, It's unneeded, since you can compute it internally?
On 2016/10/13 01:43:14, Dirk Pranke wrote: > This isn't actually unused, right? Rather, It's unneeded, since you can compute > it internally? And, more importantly, GetOutputBuildDirectory() is returning values based on assumptions (e.g., assuming //out), whereas the recipe that is passing things in knows the correct value. So, this change seems like it'd make things more fragile and is a bad idea as a result, unless I'm missing something?
On 2016/10/13 at 01:57:01, dpranke wrote: > On 2016/10/13 01:43:14, Dirk Pranke wrote: > > This isn't actually unused, right? Rather, It's unneeded, since you can compute > > it internally? I meant that whatever value passed in to --build-dir is ignored, so that value is unused. build_dir is used, but it's always gotten from build_directory.GetBuildOutputDirectory(). > And, more importantly, GetOutputBuildDirectory() is returning values based on assumptions (e.g., assuming //out), whereas the recipe that is passing things in knows the correct value. > > So, this change seems like it'd make things more fragile and is a bad idea as a result, unless I'm missing something? If GetOutputBuildDirectory() is potentially more fragile or discouraged, then do you think it sounds OK to always use the --build-dir passed in from api.chromium.c.build_dir?
On 2016/10/13 16:48:36, qyearsley wrote: > On 2016/10/13 at 01:57:01, dpranke wrote: > > On 2016/10/13 01:43:14, Dirk Pranke wrote: > > > This isn't actually unused, right? Rather, It's unneeded, since you can > compute > > > it internally? > > I meant that whatever value passed in to --build-dir is ignored, so that value > is unused. build_dir is used, but it's always gotten from > build_directory.GetBuildOutputDirectory(). Oh, I see, I missed line 275 (and the optparse help comment). Sorry! > > And, more importantly, GetOutputBuildDirectory() is returning values based on > assumptions (e.g., assuming //out), whereas the recipe that is passing things in > knows the correct value. > > > > So, this change seems like it'd make things more fragile and is a bad idea as > a result, unless I'm missing something? > > If GetOutputBuildDirectory() is potentially more fragile or discouraged, then do > you think it sounds OK to always use the --build-dir passed in from > api.chromium.c.build_dir? Yes, I would use --build-dir; I'm not sure why it was ignored, though you should to look at the file history to see.
On 2016/10/13 at 17:06:48, dpranke wrote: > On 2016/10/13 16:48:36, qyearsley wrote: > > On 2016/10/13 at 01:57:01, dpranke wrote: > > > On 2016/10/13 01:43:14, Dirk Pranke wrote: > > > > This isn't actually unused, right? Rather, It's unneeded, since you can > > compute > > > > it internally? > > > > I meant that whatever value passed in to --build-dir is ignored, so that value > > is unused. build_dir is used, but it's always gotten from > > build_directory.GetBuildOutputDirectory(). > > Oh, I see, I missed line 275 (and the optparse help comment). Sorry! > > > > And, more importantly, GetOutputBuildDirectory() is returning values based on > > assumptions (e.g., assuming //out), whereas the recipe that is passing things in > > knows the correct value. > > > > > > So, this change seems like it'd make things more fragile and is a bad idea as > > a result, unless I'm missing something? > > > > If GetOutputBuildDirectory() is potentially more fragile or discouraged, then do > > you think it sounds OK to always use the --build-dir passed in from > > api.chromium.c.build_dir? > > Yes, I would use --build-dir; I'm not sure why it was ignored, though you should > to look at the file history to see. After looking at the file history, I sort of see why it is how it is, and I also see that this is an issue that affects more than just this script, so maybe we can clean up several files at once. Filed bug http://crbug.com/655798.
Description was changed from ========== Don't pass unused parameter --build-dir to archive_layout_test_results.py. ========== to ========== Use passed-in --build-dir instead of GetBuildOutputDirectory in archive_layout_test_results.py. BUG=655798 ==========
On 2016/10/13 at 21:32:10, qyearsley wrote: > On 2016/10/13 at 17:06:48, dpranke wrote: > > On 2016/10/13 16:48:36, qyearsley wrote: > > > On 2016/10/13 at 01:57:01, dpranke wrote: > > > > On 2016/10/13 01:43:14, Dirk Pranke wrote: > > > > > This isn't actually unused, right? Rather, It's unneeded, since you can > > > compute > > > > > it internally? > > > > > > I meant that whatever value passed in to --build-dir is ignored, so that value > > > is unused. build_dir is used, but it's always gotten from > > > build_directory.GetBuildOutputDirectory(). > > > > Oh, I see, I missed line 275 (and the optparse help comment). Sorry! > > > > > > And, more importantly, GetOutputBuildDirectory() is returning values based on > > > assumptions (e.g., assuming //out), whereas the recipe that is passing things in > > > knows the correct value. > > > > > > > > So, this change seems like it'd make things more fragile and is a bad idea as > > > a result, unless I'm missing something? > > > > > > If GetOutputBuildDirectory() is potentially more fragile or discouraged, then do > > > you think it sounds OK to always use the --build-dir passed in from > > > api.chromium.c.build_dir? > > > > Yes, I would use --build-dir; I'm not sure why it was ignored, though you should > > to look at the file history to see. > > After looking at the file history, I sort of see why it is how it is, and I also see that this is an issue that affects more than just this script, so maybe we can clean up several files at once. Filed bug http://crbug.com/655798. Now updated to use the passed in value. CL description also updated.
https://codereview.chromium.org/2420543002/diff/40001/scripts/slave/chromium/... File scripts/slave/chromium/archive_layout_test_results.py (right): https://codereview.chromium.org/2420543002/diff/40001/scripts/slave/chromium/... scripts/slave/chromium/archive_layout_test_results.py:162: chrome_dir = os.path.abspath(build_dir) Doesn't this still need to be options.build_dir? https://codereview.chromium.org/2420543002/diff/40001/scripts/slave/chromium/... scripts/slave/chromium/archive_layout_test_results.py:165: options.results_dir = os.path.abspath(os.path.join(build_dir, Same. https://codereview.chromium.org/2420543002/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2420543002/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/steps.py:1704: '--build-dir', api.chromium.c.build_dir, Are you changing this to sort the parameters? If so, you might as well sort all of them. However, changing this means you also need to regenerate the recipe expectations, so you might just leave this file alone :).
https://codereview.chromium.org/2420543002/diff/40001/scripts/slave/chromium/... File scripts/slave/chromium/archive_layout_test_results.py (right): https://codereview.chromium.org/2420543002/diff/40001/scripts/slave/chromium/... scripts/slave/chromium/archive_layout_test_results.py:162: chrome_dir = os.path.abspath(build_dir) On 2016/10/13 at 23:34:43, Dirk Pranke wrote: > Doesn't this still need to be options.build_dir? Yep! Now fixed. https://codereview.chromium.org/2420543002/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_tests/steps.py (right): https://codereview.chromium.org/2420543002/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/steps.py:1704: '--build-dir', api.chromium.c.build_dir, On 2016/10/13 at 23:34:43, Dirk Pranke wrote: > Are you changing this to sort the parameters? If so, you might as well sort all of them. > > However, changing this means you also need to regenerate the recipe expectations, so you might just leave this file alone :). Yeah, that sounds good :-)
lgtm
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use passed-in --build-dir instead of GetBuildOutputDirectory in archive_layout_test_results.py. BUG=655798 ========== to ========== Use passed-in --build-dir instead of GetBuildOutputDirectory in archive_layout_test_results.py. BUG=655798 Committed: https://chromium.googlesource.com/chromium/tools/build/+/072fdd58f2070edbb909... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/tools/build/+/072fdd58f2070edbb909...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2419133002/ by qyearsley@chromium.org. The reason for reverting is: I think this could have possibly caused http://crbug.com/656063, so speculatively reverting. |
