Resurrected pants goal idea.

Review Request #695 — Created July 16, 2014 and submitted

gmalmquist
pants
garrett/idea-resurrection
349
pants-reviews
jsirois, zundel
This mostly involved updating outdated references, and making utility functions to mimick functionality refactored long ago.

Source and Javadoc jars are not mapped in, and I'm not quite sure how to get that working again. I'd appreciate any input on that matter.

The code in idea_gen and ide_gen isn't very clean, because it wasn't very clean in the first place. It would be good to go back at some point and tidy it up.

Idea gen is also much more efficient than it used to be, thanks to the round manager: it now avoids compiling or jarring anything, which saves tons of time. It would still be nice to include a flag to optionally disable codegen, but I'm not sure how (un)trivial that is at this juncture.
Added (passing) integration tests under tasks/test_idea_integration.py
Tests under tasks:integration pass.
  • 0
  • 0
  • 8
  • 0
  • 8
Description From Last Updated
ZU
  1. 
      
  2. Aside: it seems kind of weird that you have to do both round_manager.require('ivy_jar_products') and self.context.products.require_data('ivy_jar_products') Couldn't the latter imply the former? 
    1. +1, s/round_manager.require/round_manager.require_data/ for ivy_jar_products - why the 2? History - it'll get cleaned up.
  3. I think you could say 't.is_java' or t.is_jvm here instead of t.has_label...  Patrick said something about this he wanted to get rid of - I wasn't sure if was the label or the attribute.
    1. It was the attribute, because it clutters the base Target; that came up when jaxb_gen was being reviewed.
  4. Maybe change to context.log. ?
  5. @John, Garrett and I talked about this on the github review. At first he had removed the excludes as the TODO suggested, but there is still a bunch of cruft we don't want to see when doing a search.
    1. It would be nice if this could be derived.  IE: exclude .pants.d/ globally mod the stuff used - ie: source_roots of codegen synthetic targets, ie: .pants.d/gen/protoc - etc.
  6. I think this is a good start on an integration test, but I would like to eventually see some kind of comparison to make sure the project files are created correctly.  It could be a 'goldfile' comparison or an XML parse to be sure that the data looks sane.  I am not going to insist on this but if we aren't going to do it we should add a TODO 
  7. At square we are also dependent on the --idea-project-dir flag, it would be good to add a test for that (again, OK to add a TODO)
  8. 
      
JS
  1. 
      
  2. s/:/: /
  3. Why the indirection, does this suffice inlined here?:
    self._prepare_project(self.context)
    
    
  4. IIUC the .iws contains personal editor state, so using this template to emit an .iws nukes editor state history when I re-gen a project.  If so this was not the case previously and could be disruptive.  I find no editor.open_path injected or template use so maybe this can just be killed along with a few lines in idea_gen.py until the RB that intends to use this stuff.
    1. Yeah, I'll remove it. I was looking into configuring a default iws which opened the relevant sources, but realized that overwritting pre-existing iws would be bad so decided against it.
    2. Looks like its still in the review, did you remove it from git or is this a review board issue?
  5. 
      
GM
ZU
  1. 
      
  2. In this message it would be nice to suggest a root cause that would suggest an action a user can take.  Like maybe there are two source_root() definitions that overlap?  Can BUILD files do something odd with sources = directives (like use ../) to cause it?
  3. I thought you were going to remove this?
  4. I thought you were going to remove this?
  5. 
      
GM
JS
  1. A few more small things to fix or TODO/file.  I'd like to get this in after this round.
  2. src/python/pants/backend/jvm/tasks/ide_gen.py (Diff revisions 1 - 3)
     
     
    This line and next 2 have extra space padding on RHS that can be killed.
  3. This does everything you want already:
    https://github.com/twitter/commons/blob/master/src/python/twitter/common/contextutil/__init__.py#L62
    
    So perhaps below:
    with temporary_dir(root='.pants.d') as project_dir:
      # ...
  4. Unused?
    1. Not used, but I meant to use it to replace the two TaskErrors below. I'll fix that.
  5. OK - there are at least to bugs in pants itself this hacks around:
    1.) 2 or more targets owning the same absolute path
    2.) Those targets having different (or no) source roots defined
    
    It would be nice to fix those and fail or log.error here.
    I'm happy to keep this as-is for this RB, but fleshing out the very weird to a rundown of issues to TODO for fixup would be great.
    1. I'll add a TODO. An error should also be logged further down (~ line 555) if this occurs.
  6. How about inlining source_tuple.  Its only use here and the indirection hurts comprehension/readability imo.
    1. I'll do that. Previously I had been using it a lot more, but I've since culled out that code.
  7. Looks like previous and current on the next line are unused
  8. Why not $MODULE_DIR$ and can you derive the SourceRoots under .pants.d to include and use this to derive a full exclude list generically?  Happy to see a TODO + github issue for following up if this makes sense to you too.
    1. Not quite sure how to go about this (should I be scanning source sets for the pants workdir and including them individually?), but I'll add the TODO.
  9. 
      
GM
JS
  1. Last thing from me 
  2. src/python/pants/backend/jvm/tasks/ide_gen.py (Diff revisions 3 - 4)
     
     
    Un-tuple-ize, this is broken:
    >>> import os
    >>> os.path.join(('a', 'b', 'c'))
    ('a', 'b', 'c')
    >>> os.path.join('a', 'b', 'c')
    'a/b/c'
    >>>
  3. 
      
GM
JS
  1. Onward!  Thanks Garrett.
  2. 
      
IT
  1. Just had a quick look - couple of minor nits.
    1. I missed these comments while submitting - might be good to circle back for a quick clean-up RB.
  2. doesnt look like this is used?
  3. > 100 chars, split.
  4. 
      
JS
  1. In master @ https://github.com/pantsbuild/pants/commit/46b8a26f469651426c12b178b5b8d30510ed4b14
    Please mark this review as submitted.
  2. 
      
GM
Review request changed

Status: Closed (submitted)

ZU
  1. (I should be able to commit this when Garrett says he's all done)
    1. nm, I see John just did.
  2. 
      
Loading...