towards a fix for: jvm_app's bundles ain't bundling

Review Request #71 — Created March 6, 2014 and submitted

lahosken
commons
pants-reviews
jsirois, patricklaw
Fix some relative-path logic that got jiggled out of place. This fix plagiarized from JSirois.

I set up a jvm_app with bundles().add(rglobs('config/*')). I did a goal bundle... and died failing to mkdir way the heck up the dir tree:

OSError: [Errno 13] Permission denied: '/Users/lhosken/workspace/commons/dist/funbun/../../../../../../../../config'

./build-support/bin/ci.sh

learned of the existence of the darned handy BaseBuildRootTest.
  • 1
  • 0
  • 0
  • 0
  • 1
Description From Last Updated
I think this `abspath` shouldn't be here. You should ALWAYS triple think about using `abspath` in pants--it implicitly invokes `getcwd`, ... PA patricklaw
LA
  1. behold the mail thread: https://groups.google.com/forum/#!topic/pants-devel/jt5Q-iExgdw
  2. 
      
LA
JS
  1. 
      
  2. It would really be good to .add(globs(...)) and then also add a second bundle using bundle(relative_to='...').add('...').add(globs(...))
    
    Testing all 4 test the 4 standard cases.
    1. Done, maybe. Not sure I interpreted correctly.
  3. You really can kill the 2 asserts - they will have a fairly non-helpful message that will be arguably worse than the backtrace that would otherwise happen below in self.asertEquals when either app or app.bundles is None.
  4. Was this you or spurious diff?
    1. This is real. The test asserted when bundles "noticed" their source dir didn't exist.
  5. 
      
PA
  1. 
      
  2. I think this `abspath` shouldn't be here.  You should ALWAYS triple think about using `abspath` in pants--it implicitly invokes `getcwd`, and not relying on the process's cwd is a major goal in pants.  If you have a `ParseContext` available, you should know what your "current" path relative to the buildroot is, and then use that and the buildroot to construct your ultimate path.
    
    Alternatively, Fileset (generally constructed by a glob or rglob) was refactored to always know about its "current" path relative to the buildroot, so if you don't have a ParseContext and your Fileset doesn't know about its "current" path, something very weird is happening and we should figure exactly what that weirdness is.
    1. For the posixpath variant at any rate, I see this:
      # Return whether a path is absolute.
      # Trivial in Posix, harder on the Mac or MS-DOS.
      
      def isabs(s):
          """Test whether a path is absolute"""
          return s.startswith('/')
      
      Which is what I'd expect for this check - it logically should never need to no CWD - it should just be able to look at the path given and the fs.
    2. you're probably referring to the os.path.abspath in the line below and not the abspath variable on this line - in which case - agreed.
    3. Well spotted! Removed the os.path.abspath( ) and the world didn't end :-) 
  3. 
      
LA
PA
  1. Ship It!
  2. 
      
LA
  1. 
      
  2. Hmm, this blank-line deletion looks like a goof. Kicking it out of the merge
  3. 
      
LA
  1. Thanks, folks. Pushed.
  2. 
      
LA
Review request changed

Status: Closed (submitted)

Loading...