Review Request #4387 — Created Nov. 17, 2016 and discarded
|benjyw, jsirois, mateor, stuhood, zundel|
clean-alldoes not remove
dist/export-classpath, even though the exported classpaths would be invalid anyway after
clean-all. This change explicitly removes
This will also one way for IntelliJ to detect whether a
clean-allhas 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/
|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|
Revision 2 (+29 -8)
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.
I don't understand why we need this option at all.
distis the standard location for writing final products/outputs, and having fixed locations under
distfor specific outputs seems fine to me.
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.
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.
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
How about bite the bullet and delete all of pants_distdir during clean?
By all standards of reason,
distshould 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.
As commented on the pants-devel@ thread: I think clean-all should move toward cleaning the
distdirectory as well (probably with a deprecation cycle).