Review Request 4387: Have clean-all remove dist/export-classpath

[Yi Cheng]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

Review request for pants-reviews, Benjy Weinberger, John Sirois, Mateo Rodriguez, Stu Hood, and Eric Ayers. By Yi Cheng. Repository: pants Description 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 be also be one of the ways IntelliJ is going to detect whether a clean-all has happened on disk, in order to determine whether to invoke Pants under noop circumstance. Related RB: https://rbcommons.com/s/twitter/r/4363/ Testing https://travis-ci.org/pantsbuild/pants/builds/176862130 Diffs src/python/pants/backend/jvm/tasks/jvm_compile/jvm_classpath_publisher.py (3718f1f2e4a6eaebc9d344ae282f30197b55bb93) src/python/pants/core_tasks/clean.py (f77b99a66f5dbc766e237ad498400129a7025cd8) src/python/pants/option/global_options.py (4bfd448c76b94143312015cdc885581ec12a690f) tests/python/pants_test/backend/jvm/tasks/jvm_compile/test_jvm_classpath_published.py (633705bf2ed1a6989a3279c83c83c0cff1d5f2df) tests/python/pants_test/tasks/test_clean_all_integration.py (c28157ab2806f398873be4f2776e2da588572b39) View Diff

[Yi Cheng]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

Review request for pants-reviews, Benjy Weinberger, John Sirois, Mateo Rodriguez, Stu Hood, and Eric Ayers. By Yi Cheng.

Updated Nov. 17, 2016, 11:24 p.m. Changes grammar Repository: pants Description (updated)

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/

[Benjy Weinberger]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_classpath_publisher.py (Diff revision 1)
33	basedir = os.path.join(self.get_options().pants_distdir, self._output_folder)	27	basedir = self.get_options().pants_export_classpath_dir

This change is for the worse - see below.

src/python/pants/option/global_options.py (Diff revision 1)
		157	register('--pants-export-classpath-dir', advanced=True, metavar='<dir>',

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.

- Benjy Weinberger

On November 17th, 2016, 11:24 p.m. UTC, Yi Cheng wrote:

Review request for pants-reviews, Benjy Weinberger, John Sirois, Mateo Rodriguez, Stu Hood, and Eric Ayers. By Yi Cheng. Updated Nov. 17, 2016, 11:24 p.m.

Repository: pants Description

[Yi Cheng]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

Review request for pants-reviews, Benjy Weinberger, John Sirois, Mateo Rodriguez, Stu Hood, and Eric Ayers. By Yi Cheng.

Updated Nov. 18, 2016, 7:13 a.m. Changes address comment Repository: pants Description

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/

Testing (updated) https://travis-ci.org/pantsbuild/pants/builds/176937985 Diffs (updated) src/python/pants/backend/jvm/subsystems/jvm.py (28311f66aca4faa1d73782cc8b8bc134eb46637c) src/python/pants/backend/jvm/tasks/jvm_compile/jvm_classpath_publisher.py (3718f1f2e4a6eaebc9d344ae282f30197b55bb93) src/python/pants/core_tasks/clean.py (f77b99a66f5dbc766e237ad498400129a7025cd8) tests/python/pants_test/backend/jvm/tasks/jvm_compile/test_jvm_classpath_published.py (633705bf2ed1a6989a3279c83c83c0cff1d5f2df) tests/python/pants_test/tasks/test_clean_all_integration.py (c28157ab2806f398873be4f2776e2da588572b39) View Diff Show Changes

[Yi Cheng]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

On November 18th, 2016, 4:57 a.m. UTC, Benjy Weinberger wrote:

src/python/pants/option/global_options.py (Diff revision 1)
		157	register('--pants-export-classpath-dir', advanced=True, metavar='<dir>',

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.

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

- Yi

On November 18th, 2016, 7:13 a.m. UTC, Yi Cheng wrote:

Review request for pants-reviews, Benjy Weinberger, John Sirois, Mateo Rodriguez, Stu Hood, and Eric Ayers. By Yi Cheng.

Updated Nov. 18, 2016, 7:13 a.m. Repository: pants

Description 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/ Testing

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

src/python/pants/backend/jvm/subsystems/jvm.py (28311f66aca4faa1d73782cc8b8bc134eb46637c) src/python/pants/backend/jvm/tasks/jvm_compile/jvm_classpath_publisher.py (3718f1f2e4a6eaebc9d344ae282f30197b55bb93) src/python/pants/core_tasks/clean.py (f77b99a66f5dbc766e237ad498400129a7025cd8)

tests/python/pants_test/backend/jvm/tasks/jvm_compile/test_jvm_classpath_published.py (633705bf2ed1a6989a3279c83c83c0cff1d5f2df) tests/python/pants_test/tasks/test_clean_all_integration.py (c28157ab2806f398873be4f2776e2da588572b39) View Diff

[Benjy Weinberger]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

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.

src/python/pants/backend/jvm/subsystems/jvm.py (Diff revision 2)
		51	register('--classpath-export', advanced=True, metavar='<dir>',

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.

src/python/pants/core_tasks/clean.py (Diff revision 2)
		11	from pants.backend.jvm.subsystems.jvm import JVM

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.

- Benjy Weinberger

On November 18th, 2016, 7:13 a.m. UTC, Yi Cheng wrote:

Review request for pants-reviews, Benjy Weinberger, John Sirois, Mateo Rodriguez, Stu Hood, and Eric Ayers. By Yi Cheng. Updated Nov. 18, 2016, 7:13 a.m.

Repository: pants

Description 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/ Testing

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

[Mateo Rodriguez]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

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.

src/python/pants/backend/jvm/subsystems/jvm.py (Diff revision 2)
		51	register('--classpath-export', advanced=True, metavar='<dir>',

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

src/python/pants/core_tasks/clean.py (Diff revision 2)
		37	export_classpath_dir = JVM.scoped_instance(self).get_options().classpath_export

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

src/python/pants/core_tasks/clean.py (Diff revision 2)
32	pants_wd = self.get_options().pants_workdir	41	pants_wd = self.get_options().pants_workdir

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.

- Mateo Rodriguez

On November 18th, 2016, 1:13 a.m. CST, Yi Cheng wrote:

Review request for pants-reviews, Benjy Weinberger, John Sirois, Mateo Rodriguez, Stu Hood, and Eric Ayers. By Yi Cheng.

Updated Nov. 18, 2016, 1:13 a.m.

Repository: pants Description 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/ Testing

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

[Yi Cheng]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

On November 18th, 2016, 4:15 p.m. UTC, Mateo Rodriguez wrote:

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.

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

On November 18th, 2016, 4:15 p.m. UTC, Mateo Rodriguez wrote:

src/python/pants/core_tasks/clean.py (Diff revision 2)
32	pants_wd = self.get_options().pants_workdir	41	pants_wd = self.get_options().pants_workdir

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.

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

- Yi

On November 18th, 2016, 7:13 a.m. UTC, Yi Cheng wrote:

Review request for pants-reviews, Benjy Weinberger, John Sirois, Mateo Rodriguez, Stu Hood, and Eric Ayers. By Yi Cheng.

Updated Nov. 18, 2016, 7:13 a.m.

[Yi Cheng]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

On November 18th, 2016, 3:56 p.m. UTC, Benjy Weinberger wrote:

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.

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.

- Yi

[Mateo Rodriguez]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

On November 18th, 2016, 10:15 a.m. CST, Mateo Rodriguez wrote:

src/python/pants/core_tasks/clean.py (Diff revision 2)
32	pants_wd = self.get_options().pants_workdir	41	pants_wd = self.get_options().pants_workdir

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.

On November 18th, 2016, 12:45 p.m. CST, Yi Cheng wrote:

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

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

- Mateo

[Mateo Rodriguez]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

On November 18th, 2016, 10:15 a.m. CST, Mateo Rodriguez wrote:

src/python/pants/core_tasks/clean.py (Diff revision 2)
32	pants_wd = self.get_options().pants_workdir	41	pants_wd = self.get_options().pants_workdir

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.

On November 18th, 2016, 12:45 p.m. CST, Yi Cheng wrote:

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

On November 18th, 2016, 12:48 p.m. CST, Mateo Rodriguez wrote:

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

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.

- Mateo

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4387/

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).

- Stu Hood

On November 18th, 2016, 7:13 a.m. UTC, Yi Cheng wrote:

Review request for pants-reviews, Benjy Weinberger, John Sirois, Mateo Rodriguez, Stu Hood, and Eric Ayers. By Yi Cheng. Updated Nov. 18, 2016, 7:13 a.m.

Repository: pants

Description 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/ Testing

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