Have clean-all remove dist/export-classpath

Review Request #4387 — Created Nov. 17, 2016 and discarded

wisechengyi
pants
pants-reviews
benjyw, jsirois, mateor, stuhood, zundel

Previously clean-all does not remove dist/export-classpath, even though the exported classpaths would be invalid anyway after clean-all. This change explicitly removes dist/export-classpath.

This will also one way for IntelliJ to detect whether a clean-all has happened on disk, so it could determine whether to invoke Pants when no source code has changed. Related RB: https://rbcommons.com/s/twitter/r/4363/

https://travis-ci.org/pantsbuild/pants/builds/176937985

  • 3
  • 0
  • 2
  • 0
  • 5
Description From Last Updated
I don't understand why we need this option at all. dist is the standard location for writing final products/outputs, and ... BE benjyw
This is no better!! :) We don't want core tasks to depend on JVM. It means that you can never ... BE benjyw
Seems off to specificy individual projects at the top of this execute method. I would rather see a property that ... MA mateor
WI
BE
  1. 
      
  2. This change is for the worse - see below.

  3. There definitely shouldn't be anything related to a "classpath" (which is a JVM-only concept) in the global options, or anywhere outside the JVM backend.

    1. Thanks for pointing it out. I didn't think of leveraging subsystems. It is now moved under JVM.

  4. 
      
WI
BE
  1. I think you missed the point of my previous comment. In fact, this change makes things worse: Now we don't just have a semantic dependency on the concept of "classpath" in non-JVM code, but we have a literal BUILD file dependency!

    The rule should be that if it's invalid after a clean, i.e., it's not a final, standalone product, it goes under .pants.d. Otherwise it can go under dist.

    So this should simply be under .pants.d, and it'll get cleaned as usual.

    1. Thanks for the clarification. The product is definitely not final, and it was in dist probably for historical reasons. I'll move it under to .pants.d, which is a much cleaner/easy to understand solution.

  2. I don't understand why we need this option at all. dist is the standard location for writing final products/outputs, and having fixed locations under dist for specific outputs seems fine to me.

  3. src/python/pants/core_tasks/clean.py (Diff revision 2)
     
     

    This is no better!! :) We don't want core tasks to depend on JVM. It means that you can never unregister the JVM backend, even if you don't have a single .java file in your repo.

  4. 
      
MA
  1. The option seems out of scope for the JVM subsystem too. The export-classpath is a Pants and intellij concept.

    Instead, how about adding an option to clean-all that allows extending/overriding the directories that are removed? Then you can either have export-classpath as the default, or defined within an "always_clean" constant defined in the clean task. I would love for clean-all to be configurable in our pants.ini instead of the aliases that I know people are using as a proxy.

    I left some comments below that would become obsolete if you take my above suggestion.

    1. I like this idea, but a bit out of the scope of my intention. Filed https://github.com/pantsbuild/pants/issues/4077

  2. Is there a use case for ever changing the option? Why not a constant?

  3. src/python/pants/core_tasks/clean.py (Diff revision 2)
     
     

    Seems off to specificy individual projects at the top of this execute method. I would rather see a property that defines the set of "to be deleted during clean" that holds the export-classpath as the first entry.

    That would scope this change to just deleting any project in the map from under distdir, with no changes needed to the JVM subsystem, which

  4. src/python/pants/core_tasks/clean.py (Diff revision 2)
     
     

    How about bite the bullet and delete all of pants_distdir during clean?

    By all standards of reason, dist should be removed when someone orders an operation called "clean-all". I can think of arguments against this, but it seems like the right choice to me.

    1. I do not mean to overhaul the behavior of clean-all, and dist is not removed for good reasons as well (separate discussion). I'm just going to move the product of export-classpath under .pants.d for the reason Benjy mentioned, and there would be no extra JVM dependency

    2. .pants.d is for sure the way to go. I assumed that was not being done for a specific reason.

    3. although, to be clear about why I suggested clean changes, you specifically were overhauling the behavior of the clean task. It did not operate under dist, and you were adding that ability. So the conversation was definitely in scope.

  5. 
      
ST
  1. As commented on the pants-devel@ thread: I think clean-all should move toward cleaning the dist directory as well (probably with a deprecation cycle).

  2. 
      
WI
Review request changed

Status: Discarded

Loading...