Add relative_symlink to dirutil for latest run report

Review Request #2271 — Created May 27, 2015 and submitted

jrosenfield
pants
jrosenfield/sandbox
1598
1245aed...
pants-reviews
benjyw, davidt, zundel

Both run_tracker.py and reporting.py create a symlink to the latest run report called 'latest'. Previously the symlink was created with a absolute path. We have a script that captures all this information for bug reporting, so latest should be assigned a relative path.

Travis CI build green https://travis-ci.org/pantsbuild/pants/builds/64421608
Added new unit tests to test_dirutil.py

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
ZU
  1. 
      
  2. src/python/pants/util/dirutil.py (Diff revision 1)
     
     

    I know I told you to use 'is' here but I'm having a change of heart, I think == would be better. The difference lies in testing value equality verses identity equality and in this case I don't think we care about value equality. See:

    http://stackoverflow.com/questions/2988017/string-comparison-in-python-is-vs

    1. I would think == is correct and 'is' is a bug: what if the strings have the same characters but happen to occupy different memory locations? String interning might prevent this, up to a point, but that's skating on thin ice.

    2. Sorry, I meant to say we only care about value equality (not identity)

  3. 
      
JS
  1. 
      
  2. src/python/pants/goal/run_tracker.py (Diff revision 1)
     
     
    This target needs a dep on src/python/pants/util:dirutil : https://github.com/pantsbuild/pants/blob/master/src/python/pants/goal/BUILD#L85
  3. src/python/pants/util/dirutil.py (Diff revision 1)
     
     

    The outer () are not needed, os.path.isabs(...) is not compound so there is no possible confusion.

  4. src/python/pants/util/dirutil.py (Diff revision 1)
     
     
    1 more blank line here for 2 total like you did above the function: https://www.python.org/dev/peps/pep-0008/#blank-lines
  5. No need to use tempfile with tmpdir_1 as the base - just pick a stable name for simpler code here and below where you do the same.
  6. 
      
JR
JS
  1. LGTM, just want to run down the test bit.
  2. tests/python/pants_test/util/test_dirutil.py (Diff revisions 1 - 2)
     
     
    This comment is a lie now!
  3. tests/python/pants_test/util/test_dirutil.py (Diff revisions 1 - 2)
     
     

    My bad for not being more clear in my original comment. Explicitly, instead of:

    source = tempfile.mkstemp(dir=tmpdir_1)[1]
    

    Which has the ~cryptic access of slot 1, you could be just doing:

    source = os.path.join(tmpdir_1, 'source')
    

    No cryptic, and now all modeling of path structures is rooted under tmpdir_1, so arguably easier to visualize as well.

    1. Okay thanks for the clarification. I just updated the tests!

  4. 
      
DA
  1. Ship It!
  2. 
      
JR
JS
  1. Sorry - missed one last bit - bad reviewer.  I'll patch this in after this last fix.
  2. Unused - how about just emulate the style of test_relative_symlink_source_parent above.
  3. 
      
JR
JR
ZU
  1. I patched this in to master at 7da0d06. Please mark this review as submitted.

  2. 
      
JR
Review request changed

Status: Closed (submitted)

Change Summary:

7da0d06 Thanks for all the help!

Loading...