add argFileAllowed to @CmdLine and implement the arg file content reading logic

Review Request #1643 — Created Jan. 22, 2015 and submitted

jinfeng
commons
jin/add_argfile
pants-reviews
areitz, ity, jsirois, zundel

Related to https://github.com/pantsbuild/pants/issues/972

  • add argFileAllowed to @CmdLine and implement the arg file content reading logic
  • update jar-tool to support argFileAllowed for -classpath, -files, -jars arguments.
  • add unit test for OptionInfo

CRs from Farner, Levenson are pending.

  1. Built jar-tool, args, args-core locally and use them to create a jar with @argfile specified in the cmdline.

  2. locally passed: ./pants test tests/java/com/twitter/common/args:medium

  3. Travis CI passed: https://travis-ci.org/twitter/commons/builds/50248351

ZU
  1. This is pretty much the path I was thinking of when you mentioned the issue in https://github.com/pantsbuild/pants/issues/972

    @argfile is neat. I hadn't heard of that before. But we'd have to implement it in nailgun and again in jmake to get full support... Sounds like a lot of work.

    1. nailgun isn't an issue, it is not hit by argument too long error, since we pass args to nailgun via sockets.
      if java supports @argfile, it would be great, we make a small change in pants/java/executor.py, all problem resolved. but Oracle decided only javac has @argfile support, but not for java.

      so now we have to address this arg too long issue case by case. in this case, make com.twitter.common.jar.tool a bit more flexible.

  2. 
      
JS
  1. How about this: introduce an argfile attribute to @CmdLine to signal support for specifying the flag value be read from a file?

    Use of this would look like so:

      @CmdLine(name = "classpath",
          argfile = True,
          help = "A list of classpath entries. "
              + "If a -manifest is specified its contents will be used but this -classpath will "
              + "override any entry already present.")
      private final Arg<List<String>> classPath = Arg.create(null);
    

    And then from the CLI you could do the traditional:

    -classpath=a,b,c
    

    Or:

    -classpath=@/tmp/classpathfile.2sdg53
    

    With a Cmdline attribute to turn this support on the feature is:
    + opt-in, breaks no existing commmand line
    + transparent to all parsers, OptionInfo.load could transparently read the contents of the file and pass that to the underlying Parser iff argfile=true.
    + metadata is available to the help system so it can give the hint that the @<filename> form is accepted for the flag's value.

    Are you game to implement this more comprehensive fix/solution?

    1. great suggestion! in fact i'm facing problem where i'd like to use the same custom parser to parsing each line in argfile. with this suggested approach, it's seamless.

      i'll give it a try.

    2. I think we decided we didn't want to pursue the @ approach? In that case, I'd like to see this approach implemented, when I stop using nailgun (which I'd like to try), I can't run some commands like pants ubnle.

    3. The situation is different. Here we're talking about amending the twitter/common/args @CmdLine annotation with param argfile=true/false and surfacing the usage through the help system in the com.twitter.common.jar.tool (and any other jvm CLI based on twitter/common/args.

  2. 
      
JI
JS
  1. Yup - this looks about right to me Jin.  Unit tests and adding a core science tweep - Farner, Levenson, ... may be new folks that are appropriate are all that are needed.  You'll need to just ask the tweep to look at the RB offline since they won't have a RBCommons twitter team account.
  2. 
      
JI
ZU
  1. 
      
  2. will newlines be allowed in the file?

    1. the help says the format of the content has to be exactly the same as you would specify on the cmdline, so no, newline is not really allowed as delimiter.

      now I noticed newline at the end of the file is okay and given lots editors have the auto-append-newline-at-EOF feature, so I added a test with a newline to the end of the arg file.

  3. 
      
JS
  1. 
      
  2. I think just `argFile` is enough - both the query, `if option.argFile() {` and the specification `argFile = true` seem to read clearly.
  3. That's a mouthful if several arg file supporting options are present.  Just throwing out the idea of a more concise nod to support and then in the help formatter if > 0 options support argfiles, output more detailed usage help like you have here in footnote form.
  4. Any reason not to push this down into ArgumentInfo and allow positional args to come from a file too?
    1. do we have a use case for positional args coming from file? that needs more changes. positional args are parsed in ArgScanner that any arguments not starting with '-' are mapped to positional args (https://github.com/twitter/commons/blob/master/src/java/com/twitter/common/args/ArgScanner.java#L306). do we allow some positional args from @p_args.txt while some remain on cmdline?

    2. No current use case, so I'd be fine with a TODO to consider support. I do think we'd allow a mix of positional args and arg files if this gets implemented some day when @Positional(..., argFile = true). So this would be valid:

      java ... @args.txt
      

      As would:

      java ... one @args1.txt two @args2.txt
      

      And in the latter the positional args would be ['one', ...args from args1.txt..., 'two', ...args from args2.txt...]

    3. pushed to ArgumentInfo, and put todo for PositionalInfo.

  5. I always find javadoc on private members unsettling - did the author mean to make this public or protected? - how about in comment form?
    
    That said - this comment seems to add no value over the code - they are ~1-1 and I could learn all this from reading the 5 code lines.
  6. How about make this private and then drop the javadoc and the precondition check since it then is clearly all local code.
    1. made private and dropped the javadoc. Didn't remove the check, per your comment below, subclassing diff errors from IllegalArgumentException.

  7. s/ +/,/
  8. Kill extra blank line - you're in python mode here I think.
  9. FYI: http://junit.org/javadoc/latest/org/junit/rules/TemporaryFolder.html
  10. Consider OptionInfo static inner class IllegalArgumentException subclasses for the error branches so you can actually test the right branch.
    1. turns out even if i subclass, because the test uses ArgScanner().parse which catches all the optionalinfo's IllegalArgumentException and recreate a new IllegalArgumentException. So I'll keep IllegalArgumentException.

  11. 
      
JI
  1. One question came up in Twitter internal review: why do we need argFileAllowed? Why not just turn this on for all the cmd line?
    
    I can't think of many reasons other than we are cautious on limiting potential escaping problems where @ might be a valid leading character for an argument's value. By consciously turning on, we at least can evaluate the risk case by case. But this alone doesn't seem to be a very strong argument.
    
    Opinions?
    1. You named my reason.  This definitely could be a global flag or arg parser option to turn on @ support when an app knows all its options have no @ escaping issues.  This just seemed simpler based on a few guesses: the feature will be used sparingly + it is very likely only ever used on Collection types options and positional.
      
      One negative to turning this on globally is that an app needs to learn about the semantics of flags in libraries it uses.  If it uses a 3rdparty (or just developed by others in the same codebase) lib that is @ sensitive, it may not know this since the option is rarely used.
      
      My arguments are fairly weak.
  2. 
      
JI
JS
  1. Thanks for this Jin.  In case its not clear, this will need to be cherry-picked inside Twitter and then published so we can use it in pants.
    1. will check the wiki out.

  2. I won't push beyond this last nudge, but you check this on line 145 above.  If you check input parameters for private APIs and are consistent about it ... that would be alot of checks!  What singles out this internal check as especially important for example?
    1. it's slightly different. The isNullOrEmpty above on line 145 is checking the overall argument value; this line is checking the substring that strips @ from the value. This check basically catches cases like "-classpath=@" without file specified.

    2. Aha - makes sense.  In that case the check need only be argFilePath.isEmpty() since you checked null above and don't mark the parameter @Nullable here.
  3. Is this style allowed now for twitter java? As I recall despite the ability to fit on a single line the method should be layed out like getHelp directly above.

    1. I'll check.

    2. It did pass the checkstyle in science master, but for consistency I've made it multi-line.

  4. 
      
JI
DT
  1. Ship It!
  2. 
      
JI
ZU
  1. 
      
  2. Are these lists comma separated? whitespace separated? Maybe worth noting in the help.

    1. added. it's comma-separated.

  3. 
      
JI
IT
  1. thanks for adding tests!

  2. checkstyle should catch this - break it out into lines.

    1. actually I've applied patch in source/science branch off master and checkstyle went through fine. (I did then modify one line to add more than 100-char to ensure checkstyle is working, and it did)

      however, for consistency reason with the rest of the methods in the class, i've changed it to multi-line.

  3. same here, = should be on the line above

    1. no, in sciece java land, =, +, and other the operators, in case of a continuing line break, should be placed on the new line.

      this is different style guide than python style we use in pants world.

      see https://confluence.twitter.biz/display/ENG/Java+Style+Guide

  4. formatting

    1. not sure what's the problem here. you wanna me to break each argument into its ownline? This seems a bit excessive.

  5. formatting

    1. formatting is fine. this is a single line, just RB wrapped it. :)

  6. formatting

    1. this is in accordance with https://confluence.twitter.biz/display/ENG/Java+Style+Guide#indent-style.

      throw new IllegalArgumentException(
          String.format("Unable to read argument '%s' value from file '%s'.",
              optionName, argFilePath),
          e);
      

      Each continuation break has 4 space indent.

  7. s/Positional/positional arguments

  8. formatting of multi-line args throughout

    1. again, this is in accordance with this is in accordance with https://confluence.twitter.biz/display/ENG/Java+Style+Guide#indent-style..

      And all the files have passed checkstyle in a branch off science master.

    2. Are there any non-internal references to the style guide being discussed here? It's hard to CR for style when the canonical guide is Twitter-internal.

    3. https://github.com/twitter/commons/blob/master/src/java/com/twitter/common/styleguide.md
    4. Ah, thanks.

  9. 
      
IT
  1. lgtm - thanks for working through these

  2. 
      
JI
  1. this is now patched into github twitter common:

    https://github.com/twitter/commons/commit/31aa229f3f5a3d86111782b7cdcd096e8733e64b

  2. 
      
JI
Review request changed

Status: Closed (submitted)

Loading...