FBInfer for Review Bot

Review Request #11202 — Created Sept. 24, 2020 and submitted

Information

ReviewBot
master
6f9b151...

Reviewers

FBInfer Tool is now working for all the following build configurations:

  • Android
  • Apache Ant
  • Buck
  • C
  • CMake
  • Gradle with and without a Wrapper
  • iOS
  • Java
  • Make
  • Maven
  • Objective-C
  • XCode

The usability of this tool is largely dependent on the setup of a
user's ReviewBoard environment. Meaning that if a user would like to
use Review Bot to test an Android project. The user must have a working
Android SDK in the server instance in which the Review Bot is pulling
down the repository. Setups for each particular environment will be
offered in the documentation. The latest revisions include
optimizations that reduce the usage of infer run. So that it is
executed minimally, to enhance speed and efficiency.

Each build configuration was tested manually with its own environment
and repository. More information about the tests can be found in
Week 3 and Week 4 of my Notion journal: https://bit.ly/351ttRQ.


Description From Last Updated

Can you update the screenshot to show the latest text?

daviddavid

Summary should say "Review Bot", rather than "ReviewBot." We've altered the name a bit, but still have places where we …

chipx86chipx86

Something seems to have gone wrong with the hange description. Guessing it's because there's no blank line before or after …

chipx86chipx86

F401 'reviewbot.utils.process.execute' imported but unused

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E124 closing bracket does not match visual indentation

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F401 'reviewbot.utils.process.execute' imported but unused

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

Could you add a comment at the start of the file? e.g. Following the doc8.py, """Review Bot tool to run …

ceciliaweiceciliawei

Yep, that is definitely an artifact from shuffling my code around. Good catch, seems that flake8 did not complain about …

jblazusijblazusi

Were you intended to put the command in the end? Shouldn't it just be list.extend(...)?

ceciliaweiceciliawei

Since this change is in another CR for review, is it possible to remove the change from this CR? So …

ceciliaweiceciliawei

I was reading something related to json then I thought about this code review. Would it be helpful to put …

ceciliaweiceciliawei

This should probably be dedented one space.

daviddavid

This should use single quotes.

daviddavid

Let's use double quotes around "build"

daviddavid

Let's add a trailing comma to this line.

daviddavid

This should use single quotes.

daviddavid

Let's add a trailing comma to this line.

daviddavid

This should use single quotes.

daviddavid

Let's use double quotes around "iphonesimulator", which lets us avoid the \\ characters.

daviddavid

Let's add a trailing comma to this line.

daviddavid

Can you add periods to the end of the comments in here? This line and others below.

daviddavid

Let's add a trailing comma to this line.

daviddavid

There are a bunch of places where we use settings['build_type']. Perhaps pull that out into a build_type variable?

daviddavid

Let's add a trailing comma to this line.

daviddavid

Please add a blank line between these two.

daviddavid

Add a blank line between these two please.

daviddavid

The comma within the function call here can go away.

daviddavid

I don't think we need to list the file here, since the diff comment will be attached to the file …

daviddavid

Just for consistency, can we call the entry point fbinfer?

daviddavid

I just realized, we should use shlex.split(build_type) here, to parse it as a shell would. Otherwise if there are quoted …

daviddavid

"fbinfer" or "FBInfer"?

chipx86chipx86

How about putting the string inside of the () on the next line, indented 4 spaces? That'll give it breathing …

chipx86chipx86

This can be one statement.

chipx86chipx86

You'll want a try/except around this, since it can fail.

chipx86chipx86

Typo: "compilation". The comment could use an adjustment. I wasn't sure at first glance what it was saying.

chipx86chipx86

How about: build_type = 'make' settings['build_type'] = build_type This is a bit cleaner, since you're not having to store a …

chipx86chipx86

It's best to use += instead of .extend(). Operators are faster to invoke than functions.

chipx86chipx86

Better to use += here too.

chipx86chipx86

And here.

chipx86chipx86

This will need a try/except.

chipx86chipx86

This becomes a bit easier to see as one concrete operation if you do: execute(['infer', 'run', '--'] + shlex.split(build_type) + …

chipx86chipx86

+= here as well.

chipx86chipx86

This needs a try/except.

chipx86chipx86

fp is the standard variable for a file pointer when opening a file. You should also specify an explicit mode …

chipx86chipx86

Can we break after this match?

chipx86chipx86

Formatting will be more consistent with our standards by doing: f.comment('Bug Type: %s\n%s: %s' % (entry['bug_type_hum'], entry['severity'] entry['qualifier']), entry['line']) Or, …

chipx86chipx86

No blank line here.

chipx86chipx86

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

If you make the change to handle_files that I suggested below, then cmake should be added to this list as …

daviddavid

By my reading of the code, if the build type is one of multi_file_build_types, we'll never load the report and …

daviddavid

Would it be better to initialize build_type in __init__ instead of defining it in the method?

ceciliaweiceciliawei

Same comment as in the other CR.

ceciliaweiceciliawei

^

ceciliaweiceciliawei

^

ceciliaweiceciliawei
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

jblazusi
Review request changed
Commit:
12d61818c8fb763755274cc4d11c03981905235a
8c6e253b460c7e78839d02260874e3096e34af91

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

jblazusi
Review request changed
Commit:
8c6e253b460c7e78839d02260874e3096e34af91
bcd3d151388605cb49da1482c3d915c3de48c42f

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

jblazusi
jblazusi
jblazusi
  1. 
      
    1. Fixing the wrapping of the description to be between 70-75 characters.

  2. 
      
jblazusi
ceciliawei
  1. 
      
  2. bot/reviewbot/tools/fbinfer.py (Diff revision 4)
     
     
    Show all issues

    Could you add a comment at the start of the file? e.g. Following the doc8.py,

    """Review Bot tool to run doc8."""
    
    from __future__ import unicode_literals
    
    import logging
    
    from reviewbot.tools import Tool
    from reviewbot.utils.process import execute, is_exe_in_path
    
    1. Actually, looks like this is not consistent among the tools.

    2. This is a great suggestion, and yes, we definitely want that!

      A lot of code in our codebases predate our modern doc standards. When appropriate, we try to update existing code (for instance, we'll add a docstring to a file being modified if it's missing one, or modernize a function's docs if we're adding a new argument). All new code needs to follow the modern doc conventions, which will over time help lead to consistency.

    3. I included this """Review Bot tool to run FBInfer.""" at the top of the FBInfer file. Not sure if anything else needs to be included.

  3. 
      
jblazusi
  1. 
      
  2. 
      
jblazusi
jblazusi
jblazusi
jblazusi
ceciliawei
  1. 
      
  2. bot/reviewbot/tools/fbinfer.py (Diff revision 5)
     
     
    Show all issues

    Were you intended to put the command in the end? Shouldn't it just be list.extend(...)?

    1. I meant comma rather than command. Sorry about the confusion.

  3. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Show all issues

    Since this change is in another CR for review, is it possible to remove the change from this CR? So that the description could reflect the content better.

    1. I was able to fix this issue with Christian's help.

  4. 
      
ceciliawei
  1. 
      
  2. 
      
jblazusi
  1. 
      
  2. bot/reviewbot/tools/fbinfer.py (Diff revision 5)
     
     
    Show all issues

    Yep, that is definitely an artifact from shuffling my code around. Good catch, seems that flake8 did not complain about this.

  3. 
      
jblazusi
jblazusi
jblazusi
ceciliawei
  1. 
      
  2. bot/reviewbot/tools/fbinfer.py (Diff revision 7)
     
     
    Show all issues

    I was reading something related to json then I thought about this code review. Would it be helpful to put the json.load(file) in a try/catch block and add loggings? e.g.
    try: # json.load except Exception as e: logger.exception('Could not read the json: %s', e)

    1. I think that this could potentially help with tracing errors, especially if FBInfer made significant changes to their commands or perhaps generates bad json output.

  3. 
      
jblazusi
  1. 
      
    1. Included a try and except block for loading the json as an extra pre-caution

  2. 
      
jblazusi
david
  1. This is looking great. There are a bunch of comments here but they're pretty much all just style nitpicks.

  2. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    This should probably be dedented one space.

  3. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    This should use single quotes.

  4. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    Let's use double quotes around "build"

  5. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    Let's add a trailing comma to this line.

  6. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    This should use single quotes.

  7. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    Let's add a trailing comma to this line.

  8. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    This should use single quotes.

  9. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
     
    Show all issues

    Let's use double quotes around "iphonesimulator", which lets us avoid the \\ characters.

  10. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    Let's add a trailing comma to this line.

  11. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    Can you add periods to the end of the comments in here? This line and others below.

  12. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    Let's add a trailing comma to this line.

  13. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    There are a bunch of places where we use settings['build_type']. Perhaps pull that out into a build_type variable?

  14. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    Let's add a trailing comma to this line.

  15. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
     
    Show all issues

    Please add a blank line between these two.

  16. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
     
    Show all issues

    Add a blank line between these two please.

  17. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
    Show all issues

    The comma within the function call here can go away.

  18. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
    Show all issues

    I don't think we need to list the file here, since the diff comment will be attached to the file already.

  19. bot/setup.py (Diff revision 8)
     
     
    Show all issues

    Just for consistency, can we call the entry point fbinfer?

  20. 
      
jblazusi
david
  1. 
      
  2. Show all issues

    Can you update the screenshot to show the latest text?

  3. bot/reviewbot/tools/fbinfer.py (Diff revision 9)
     
     

    Is this a long enough timeout?

    1. No, I do not think that this is long enough for a timeout, so I updated it to 90.

  4. 
      
jblazusi
jblazusi
david
  1. 
      
  2. bot/reviewbot/tools/fbinfer.py (Diff revision 10)
     
     
    Show all issues

    I just realized, we should use shlex.split(build_type) here, to parse it as a shell would. Otherwise if there are quoted arguments it might not be correct.

    1. Fantastic, I did not know that something like that existed. To verify that it works, I ran some manual tests, just to be certain.

  3. 
      
jblazusi
chipx86
  1. 
      
  2. Show all issues

    Summary should say "Review Bot", rather than "ReviewBot." We've altered the name a bit, but still have places where we have the old version.

  3. Show all issues

    Something seems to have gone wrong with the hange description. Guessing it's because there's no blank line before or after the list, which is needed for Markdown so it doesn't turn into italics.

  4. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    "fbinfer" or "FBInfer"?

    1. It should be FBInfer, thank you.

  5. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
     
     
     
     
    Show all issues

    How about putting the string inside of the () on the next line, indented 4 spaces? That'll give it breathing room. Like:

    {
        ...
        'help_text': (
            'Include a ...'
            '...'
        ),
    }
    
  6. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
    Show all issues

    This can be one statement.

  7. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    You'll want a try/except around this, since it can fail.

  8. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    Typo: "compilation".

    The comment could use an adjustment. I wasn't sure at first glance what it was saying.

  9. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
     
    Show all issues

    How about:

    build_type = 'make'
    settings['build_type'] = build_type
    

    This is a bit cleaner, since you're not having to store a value and then look it up immediately after.

  10. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    It's best to use += instead of .extend(). Operators are faster to invoke than functions.

    1. I did not think of that, but that totally makes sense. I'll make sure to utilize this in the future. Thank you.

  11. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    Better to use += here too.

  12. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    And here.

  13. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    This will need a try/except.

  14. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This becomes a bit easier to see as one concrete operation if you do:

    execute(['infer', 'run', '--'] +
            shlex.split(build_type) +
            [path])
    
  15. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    += here as well.

  16. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    This needs a try/except.

  17. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    fp is the standard variable for a file pointer when opening a file.

    You should also specify an explicit mode parameter.

    One last thing: This can fail (due to non-existence or permissions issues), so you will need to capture exceptions here. Maybe move this into the try.

    1. Thank you, for letting me know about this. I had no idea, I thought I was safe from errors with the with keyword, but thats only to make sure that the file will be closed.

  18. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
    Show all issues

    Can we break after this match?

    1. Unfortunately, we cannot because of the way the output is formatted. A file may have multiple errors in different JSON-formatted error objects.

  19. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
     
     
     
     
     
     
    Show all issues

    Formatting will be more consistent with our standards by doing:

    f.comment('Bug Type: %s\n%s: %s'
              % (entry['bug_type_hum'],
                 entry['severity']
                 entry['qualifier']),
              entry['line'])
    

    Or, maybe better:

    f.comment('Bug Type: %(bug_type_hum)s\n'
              '%(severity)s %(qualifier)s'
              % entry,
              entry['line'])
    

    By the way, is it bug_type_hum or bug_type_num?

    1. Interesting, I did not know that sort of %-formatting was possible.
      I know it looks like a mistake, but it is bug_type_hum. I definitely second looked as well. :)

  20. bot/reviewbot/tools/fbinfer.py (Diff revision 11)
     
     
     
     
    Show all issues

    No blank line here.

  21. 
      
jblazusi
Review request changed
Change Summary:

Addressed comments made by Christian.

Summary:
FBInfer for ReviewBot
FBInfer for Review Bot
Description:
~  

FBInfer Tool is now working for all the following build configurations:

~   * Android
~   * Apache Ant
~   * Buck
~   * C
~   * CMake
~   * Gradle with and without a Wrapper
~   * iOS
~   * Java
~   * Make
~   * Maven
~   * Objective-C
~   * XCode
~   The usability of this tool is largely dependent on the setup of a
  ~

FBInfer Tool is now working for all the following build configurations:

  ~
  ~
  • Android
  ~
  • Apache Ant
  ~
  • Buck
  ~
  • C
  ~
  • CMake
  ~
  • Gradle with and without a Wrapper
  ~
  • iOS
  ~
  • Java
  ~
  • Make
  ~
  • Maven
  ~
  • Objective-C
  ~
  • XCode
  +
  +

The usability of this tool is largely dependent on the setup of a

    user's ReviewBoard environment. Meaning that if a user would like to
~   use ReviewBot to test an Android project. The user must have a working
~   Android SDK in the server instance in which the ReviewBot is pulling
  ~ use Review Bot to test an Android project. The user must have a working
  ~ Android SDK in the server instance in which the Review Bot is pulling
    down the repository. Setups for each particular environment will be
    offered in the documentation.
    The latest revisions include optimizations that reduce the usage of
    infer run. So that it is executed minimally, to enhance speed and
    efficiency.

Commit:
ec0d93f6f13c5df3a0dcf49b57a256f2d18f6a5d
cc771a368ba1bb4f7135e6adfb05f2e23c5c2117
People:

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

jblazusi
david
  1. I just noticed a pretty significant bug with the multi_file_build_types. It will require some reworking, but other than that much of this implementation looks very solid.

    1. You are correct David and this is a blunder of mine between revisions 9-11. When I was including try/except as Christian had recommended, I had mixed up the indentation and therefore the logic of the Tool. However, I do think your recommendation is best, organizing the methods this way will make it easier to read the code and harder to make the same sort of indentation error down the road.

      In addition, I have noticed that the text can use markdown, so I have enabled rich_text=True in f.comment()

  2. bot/reviewbot/tools/fbinfer.py (Diff revision 13)
     
     
    Show all issues

    If you make the change to handle_files that I suggested below, then cmake should be added to this list as well.

  3. bot/reviewbot/tools/fbinfer.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    By my reading of the code, if the build type is one of multi_file_build_types, we'll never load the report and create comments.

    I think perhaps we should split things up so that this method is very simple:

    if settings['build_type'] in self.multi_file_build_types:
        self.run_multi_file_build(files)
    else:
        for f in files:
            self.run_single_file_build(f)
    

    We can then have run_multi_file_build do all the infer_cmd work that's done above here (plus actually load report.json and create comments for any files that are changed), and run_single_file_build would look a lot like what your current handle_file implementation looks like.

  4. 
      
jblazusi
ceciliawei
  1. 
      
  2. bot/reviewbot/tools/fbinfer.py (Diff revision 14)
     
     
    Show all issues

    Would it be better to initialize build_type in __init__ instead of defining it in the method?

    1. I am not sure, it would largely depend on when __init__ is called. It would need to be called after configurations are set and everytime afterwards, so that it is always consistent with the settings. I suppose my question would be for anyone that can answer: Is __init__ called during ./setup develop and when the reviewbot workers refresh or everytime the API is called?

      This variable is mostly used to make the code more readable and quickly reference the build_type when calling helper functions.

  3. bot/reviewbot/tools/fbinfer.py (Diff revision 14)
     
     
    Show all issues

    Same comment as in the other CR.

  4. bot/reviewbot/tools/fbinfer.py (Diff revision 14)
     
     
    Show all issues

    ^

  5. bot/reviewbot/tools/fbinfer.py (Diff revision 14)
     
     
    Show all issues

    ^

  6. 
      
jblazusi
david
  1. Ship It!
  2. 
      
jblazusi
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (07f5fd4)