Improve --help documentation and add --list-plans to show available benchmarks.
Created attachment 323121 [details] New output for --help and --list-plans For ease of review, attachment showing what the new --help and --list-plams output looks like.
Created attachment 323122 [details] Patch
Comment on attachment 323122 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323122&action=review > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:64 > + @staticmethod > + def available_plans(): I don't think this function belongs to benchmark_runner class. This class shouldn't know anything about where plan files are located. We should keep it in run_benchmark.py. > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:66 > + plans = [plan_file.replace(".plan", "") for plan_file in os.listdir(plan_directory) if plan_file.endswith(".plan")] We should continue to use os.path.splitext(plan_file)[0] This code replaces any string of ".plan" in its name like "x*.plan*ebench.plan". > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:88 > - raise Exception('Cant find any .plan file in directory %s' % plandir) > + raise Exception('Cant find any .plan file in directory %s' % BenchmarkRunner.available_plans()) I don't think this change makes sense. This error messages is about not being able to find any plans. We should show the directory name instead.
Dewei, please take a second look at Maciej's patch to make sure landing this patch won't break bots.
(In reply to Ryosuke Niwa from comment #3) > Comment on attachment 323122 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323122&action=review > > > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:64 > > + @staticmethod > > + def available_plans(): > > I don't think this function belongs to benchmark_runner class. This class > shouldn't know anything about where plan files are located. > We should keep it in run_benchmark.py. I added it here because benchmark_runner already knows where plan files are located via _find_plan_file(). I agree the knowledge should only be in one place, it seemed easier to put it in BenchmarkRunner than in run_benchmark.py. If you think it's a better direction for run_benchmark.py be the only place that knows about plan files instead, I can change things towards that direction instead. I put the code in benchmark_runner before I noticed that run_benchmark was already enumerating all plans. > > > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:66 > > + plans = [plan_file.replace(".plan", "") for plan_file in os.listdir(plan_directory) if plan_file.endswith(".plan")] > > We should continue to use os.path.splitext(plan_file)[0] > This code replaces any string of ".plan" in its name like > "x*.plan*ebench.plan". Will do. > > > Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:88 > > - raise Exception('Cant find any .plan file in directory %s' % plandir) > > + raise Exception('Cant find any .plan file in directory %s' % BenchmarkRunner.available_plans()) > > I don't think this change makes sense. This error messages is about not > being able to find any plans. > We should show the directory name instead. You're right, that makes no sense. Will revert.
(In reply to Maciej Stachowiak from comment #5) > (In reply to Ryosuke Niwa from comment #3) > > Comment on attachment 323122 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=323122&action=review > > > > > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:64 > > > + @staticmethod > > > + def available_plans(): > > > > I don't think this function belongs to benchmark_runner class. This class > > shouldn't know anything about where plan files are located. > > We should keep it in run_benchmark.py. > > I added it here because benchmark_runner already knows where plan files are > located via _find_plan_file(). I agree the knowledge should only be in one > place, it seemed easier to put it in BenchmarkRunner than in > run_benchmark.py. If you think it's a better direction for run_benchmark.py > be the only place that knows about plan files instead, I can change things > towards that direction instead. I put the code in benchmark_runner before I > noticed that run_benchmark was already enumerating all plans. Oh, okay. I guess keeping it in benchmark_runner would be okay then.
Committed r223341: <https://trac.webkit.org/changeset/223341>
<rdar://problem/35003097>