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

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

jinfeng
pants
f272215...
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",
[HTML_REMOVED] 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

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
JI
JI
ZU
  1. 
      
  2. Shouldn't this argument be mandatory? It doesn't have a default and somehow I doubt that the task will work with the target set to None. Probably it should raise an exception in init() if there isn't a valid target.

    It would be great if there were an example checked in to pants so we coul test it out.

    1. how to specify an arg is required? if no such an option, I'll add a check in the init().

      agree we should have tests. i'll file an issue to track that, right now we need to unblock our release.

    2. I'm not sure there is a way to programmatically do it, but you could add the text "mandatory" or "required" in the help text to help get the point across.

  3. 
      
IT
  1. lets ship so we can unblock and follow up rightaway with tests as Eric suggested.

  2. 
      
JI
JI
ZU
  1. Ship It!

  2. 
      
JI
  1. committers, please patch it in. thanks!

    1. Committed @ 0ee4533aa745d7c48fff14dc839ed08bea499f8e

  2. 
      
JI
Review request changed

Status: Closed (submitted)

BE
  1. 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.

  2. 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/

  3. 
      
Loading...