Fix the regression introduced in https://rbcommons.com/s/twitter/r/1186/

Review Request #1254 - Created Oct. 30, 2014 and submitted

Information
Jin Feng
pants
f272215...
Reviewers
pants-reviews
benjyw, dturner-tw, ity, zundel

The original code in benchmark_run.py was:

option_group.add_option(mkflag("target"), dest="target_class", action="append",
  help="Name of the benchmark class.")
...
...
self.caliper_args = self.context.options.target_class
self.caliper_args += [ str1, str2 ]
self.caliper_args.extend([str3, str4])
...
...
# eventually deep in the callstack, a maybe_list(caliper_args) is called.

For whatever reason, we used "append" and the target_class is a list (even though
it should be a single string argument). But anyhow, it works, since the it was an
assignment "self.caliper_args = self.context.options.target_class"

Now the change introduced in the RB, while keeping the target still a list, it
changes the line "self.caliper_args = self.context.options.target_class" to

self.args.insert(0, self.get_options().target)

Note even though it's called "target", it's actually an list. Thus we're inserting
a list as an element into a list. Later the maybe_list failed.

  • also fixed the pants.ini benchmark setting name mismatch.

local benchmark run worked.

CI (with all 3 commits) is baking: https://travis-ci.org/jinfeng/jinfeng-pants-fork/builds/39519055

Issues

  • 0
  • 1
  • 0
  • 1
Description From Last Updated
Jin Feng
Jin Feng
Eric Ayers
Ity Kaul
Jin Feng
Jin Feng
Eric Ayers
Jin Feng
Jin Feng
Review request changed

Status: Closed (submitted)

Benjy Weinberger

Thanks for fixing! I thought there was something fishy about that flag being 'append', which should have alerted me to be more careful with the value.

Under the new options system (once we switch to the new-style parser) this will simply be --target (in the bench scope):

pants bench --target

Probably best to adjust the message now, even if it's temporarily stale, as we'll never remember to fix it later...

  1. do we have to (I mean the new parser)...? this round of changes makes lots of internal scripts / CIs broken. another set of changes would add more pains.

  2. Do we have to switch to the new parser? Yes, this has been in the works for months.

    Keep in mind that the old-style flags will still work (e.g., --bench-target in this case), they just aren't the preferred way to specify them in new scripts etc. But there will be a long period where we support them, possibly forever.

  3. https://rbcommons.com/s/twitter/r/1258/

Loading...