Fix compile clean by laughedelic · Pull Request #3678 · sbt/sbt

laughedelic · 2017-10-27T10:21:05Z

Fixes #3542. I started it at the Lambda World Spree with great help from @jvican.

This is a simple ah-hoc solution, it could be improved in many ways. The main change here is that

compile:compile::clean cleans up only things in compile:classDirectory
test:compile::clean cleans up only things in test:classDirectory

Also I'm not sure how it should handle generated sources/resources.

* parametrized clieanFilesTask by the files it should delete * added a cleanFiles override for cleaning only classDirectory

jvican

LGTM! Great work @laughedelic.

jvican · 2017-10-27T10:22:10Z

main/src/main/scala/sbt/Defaults.scala

  lazy val outputConfigPaths = Seq(
    classDirectory := crossTarget.value / (prefix(configuration.value.name) + "classes"),
    target in doc := crossTarget.value / (prefix(configuration.value.name) + "api")
+  ) ++ inTask(compile)(


To reviewers: this is scoped in the default configs Compile, Test and Runtime.

I should also add cleanKeepFiles here and fix dependency on it in cleanFilesTask, because otherwise user cannot override it in this particular scope.

jvican · 2017-10-27T10:24:51Z

sbt/src/sbt-test/actions/compile-clean/test

+# it should clean only class files
+$ absent target/scala-2.12/classes/A.class
+
+> test:compile::clean


👍 great test

laughedelic · 2017-10-27T10:28:24Z

main/src/main/scala/sbt/Defaults.scala


  lazy val projectTasks: Seq[Setting[_]] = Seq(
-    cleanFiles := cleanFilesTask.value,
+    cleanFiles := Def.taskDyn { cleanFilesTask(managedDirectory.value, target.value) }.value,


This is the same as it was before. But I'm not sure if I should add managedDirectory to cleanFiles in the compile task above.

dwijnand · 2017-10-27T10:26:35Z

main/src/main/scala/sbt/Defaults.scala

  lazy val outputConfigPaths = Seq(
    classDirectory := crossTarget.value / (prefix(configuration.value.name) + "classes"),
    target in doc := crossTarget.value / (prefix(configuration.value.name) + "api")
+  ) ++ inTask(compile)(


No if you look at #3542 it talks about creating a compile:clean, not compile::clean, i.e a clean scoped to the Compile configuration, not to the compile task.

I changed that in the ticket and advised Alexey to do it this way. I think that scoping in the task makes more sense because you may be interested in deleting test class files as well.

I think that scoping in the task makes more sense because you may be interested in deleting test class files as well.

I agree, and test:clean should do that.

I'm fine with either way. I think scoping by task makes sense if there will be more variants of clean. But if it addresses only compilation task, I agree with @dwijnand that it's better to scope it by configuration. Because compile::clean gives a false impression that it should magically work with other tasks (cleaning their specific artifacts).

Because compile::clean gives a false impression that it should magically work with other tasks (cleaning their specific artifacts).

Exactly.

Hmm, test:clean is not the same to test:compile::clean. The latter is explicit and portrays better the original intent: only deleting class files. test:clean already has a meaning now and is not clear about what it should clean, so I strongly favor test:compile::clean.

Because compile::clean gives a false impression that it should magically work with other tasks (cleaning their specific artifacts).

Well, this already happens with test:clean. You would expect sbt to only delete things generated by the Test configuration, but it will delete things generated by Compile and Test.

test:clean already works now and is not clear about what it should clean

Well, this already happens with test:clean. You would expect sbt to only delete things generated by the Test configuration, but it will delete things generated by Compile and Test.

I'm not sure what you mean by "works" then. Currently test:clean just delegates to clean. Thus #3542.

There is actually a sys property to disable that delegation and I was playing with it the other day:

sbt/main/src/main/scala/sbt/internal/Act.scala

Line 446 in 05c2c50

if (java.lang.Boolean.getBoolean("sbt.cli.nodelegation")) data.getDirect(scope, key)

It's interesting because it mean that test:clean wouldn't run because it wouldn't delegate to clean, but test would continue to delegate to test:test and therefore run. I want to revisit it for sbt 2.

I'm not sure what you mean by "works" then. Currently test:clean just delegates to clean. Thus #3542.

Yup, I agree. This is why I said:

this already happens with test:clean. You would expect sbt to only delete things generated by the Test configuration, but it will delete things generated by Compile and Test.

That sys property is interesting, didn't know it.

I'm trying to think how we solve this puzzle to get the intended behaviour in test:compile::clean.

It turns out we've been missing that the actual implementation does only delete test class files, so we're good. Turning the test into the following makes the test pass:

> test:compile $ exists target/scala-2.12/resolution-cache/ $ exists target/streams/ $ exists target/scala-2.12/classes/A.class $ exists target/scala-2.12/test-classes/B.class > test:compile::clean $ exists target/scala-2.12/resolution-cache/ $ exists target/streams/ # it should clean only test class files $ exists target/scala-2.12/classes/A.class $ absent target/scala-2.12/test-classes/B.class > compile:compile::clean $ exists target/scala-2.12/resolution-cache/ $ exists target/streams/ # it should clean only compile class files $ absent target/scala-2.12/classes/A.class $ absent target/scala-2.12/test-classes/B.class

There is actually a sys property to disable that delegation and I was playing with it the other day

Wow! I just told @jvican yesterday that delegation is confusing and I'd like sbt to fail when I mix up scopes and get a delegated value without realising it.

It turns out we've been missing that the actual implementation does only delete test class files, so we're good.

👍 done in f2aab78

dwijnand · 2017-10-27T10:28:41Z

sbt/src/sbt-test/actions/compile-clean/test

+$ exists target/scala-2.12/resolution-cache/
+$ exists target/streams/
+# it should clean both compile and test classes
+$ absent target/scala-2.12/classes/A.class


I have 2 concerns with this test: (1) I think it's over-specifying - it shouldn't make assertions about whether or not there is a streams directory, etc, and (2) I also think it's wrong - cleaning test classes shouldn't clean compile classes too.

I agree. Ideally, only tests' class files should be removed.

Can you elaborate how you would implement this so that cleaning test classes shouldn't clean compile classes? I don't think there's a way to avoid that given how sbt deals with configuration dependencies.

I don't know, I would have to experiment.

it shouldn't make assertions about whether or not there is a streams directory, etc

@dwijnand OK. What about resolution_cache? I just want to check that whatever else is generated by sbt is kept.

Same with resolution_cache. I agree with the intent. My idea was something along the lines of:

test:compile # sanity check exists classes exists test-classes compile:clean absent classes exists test-classes test:compile test:clean exists classes absent test-classes

OK. How about creating a file in target/ and checking that it's not deleted? (because normal clean would delete it).

Also I have to check for existence of specific class files, because the root class folders are preserved.

OK. How about creating a file in target/ and checking that it's not deleted? (because normal clean would delete it).

Sounds good.

Also I have to check for existence of specific class files, because the root class folders are preserved.

Yep - I was just using short-hand.

Addressed in 4bba70b

To check that test:compile::clean deletes only test-generated classes

dwijnand · 2017-10-27T16:30:31Z

main/src/main/scala/sbt/Defaults.scala

  }

  /** Implements `cleanFiles` task. */
-  def cleanFilesTask: Initialize[Task[Vector[File]]] =


Can't get rid of this, for binary compatibility (MiMa-enforced) reasons.

Does f486d9d fix it?

Travis CI will let us know.

Travis is green ✅

laughedelic · 2017-10-28T17:36:59Z

@dwijnand @jvican
I implemented (and tested) same change for

{compile,test}:{managedSources,managedResources}::clean

to clean only generated (re)sources.

Can I add it here or should I submit a separate PR?

dwijnand · 2017-10-30T07:44:50Z

I'd hold fire on any additional feature for the moment (obviously hold onto those SHAs!): this is still not implemented as compile:clean and test:clean.

jvican · 2017-10-30T08:02:46Z

I'm against that. I've already explained why in this PR several times. Can you elaborate on why only class files should be cleaned by configurations?

laughedelic · 2017-11-01T21:28:38Z

I didn't have any strong opinion about this in the beginning, but over the couple of days at Lambda World I talked with @jvican and he basically opened my eyes on some basic things about scoping in sbt that I didn't understand (or understood wrong) before.

I think that this should be scoped by task, because it makes sense conceptually: you clean only the products of this task (in whatever configuration scope). And the source/resource-generation tasks mentioned above are another example where clean could be useful and if it's not scoped by task, how do you distinguish it from the compile task scoped one.

Another thing is the concern from #3678 (comment):

Because compile::clean gives a false impression that it should magically work with other tasks (cleaning their specific artifacts).

but I realised that this is something general about sbt design: because of the scope delegation user should just never have such expectation and always inspect keys he uses to see if they are actually defined in the given scope or delegated to some other.

eed3si9n

I also agree with @dwijnand that the scoping of additional clean should be configuration like Compile and Test.
After all, that's what the expectation of #3542 stated:

compile:clean should clean only the compilation artifacts.

jvican · 2018-05-31T13:08:39Z

compile:clean will clean not only class files but any task result produced by the configuration Compile. The ticket I opened in #3542 mentions only compilation artifacts, the compilation artifacts are only the class files and analysis files.

eed3si9n · 2018-06-01T17:06:04Z

I agree it's not perfect, but compilation could happen as part of run or test, so I don't think there's a point in scoping it specific to compile. If we are being super specific, I'm guessing that these files are generated in all sorts of subtasks under compile too.

dwijnand · 2018-06-01T17:14:36Z

See also @retronym's super simple cleanClasses impl: scala/scala#6256 (comment)

jvican · 2018-06-01T21:12:43Z

but compilation could happen as part of run or test

Wait, even if compilation happens because the user does run or test, how does that change the fact that compile::clean will only clean class files and analysis files? Surely we're not talking about having the same clean task for run or test, so I don't think the argument makes a difference.

Maybe someone can explain to me what's the value of having clean be scoped to the Compile configuration instead of the compile? I'm really missing the point. Also, what happens with any other configuration that triggers compile, like IntegrationTest or Test or Jmh?

eed3si9n · 2018-06-13T05:41:11Z

Maybe someone can explain to me what's the value of having clean be scoped to the Compile configuration instead of the compile?

For me it comes down to usability and symmetry. Configuration scoping, in general I think is easier to understand for users.

If you want to add scalacOptions in Compile configuration we do:

Compile / scalacOptions += "-deprecation"

Same goes for lots of other settings related to compilation. None of them are scoped to compile task. If you want your Compile products to be cleaned it should naturally be:

> Compile/clean

If you want to compile or clean things in Test scope:

> Test/compile
> Test/clean

jvican · 2018-08-13T12:40:41Z

@eed3si9n But note that your proposal is not at odds with mine, we can still scope it in the compile task, and then delegate to it from the configuration scope. The idea is that the configuration scope, in the future, removes all products produced by other tasks scoped in Compile, whereas compile::clean is strictly charged with removing compilation products. This benefits people like me that only want the class files to be removed, and benefits other plugin authors too that may want to enrich Compile/clean with cleaning resources generated by their plugins.

eed3si9n · 2019-03-22T20:07:49Z

Closing this in favor of 172c8e9.

laughedelic added 3 commits October 27, 2017 11:04

Added a test for compile::clean

b28827c

A simple implementation of compile::clean

85cef2f

* parametrized clieanFilesTask by the files it should delete * added a cleanFiles override for cleaning only classDirectory

Added a test for test:compile::clean

b26a27c

eed3si9n added the ready label Oct 27, 2017

dwijnand added this to the 1.1.0 milestone Oct 27, 2017

laughedelic mentioned this pull request Oct 27, 2017

Make compile:clean clean only compilation artifacts #3542

Closed

jvican approved these changes Oct 27, 2017

View reviewed changes

laughedelic commented Oct 27, 2017

View reviewed changes

dwijnand requested changes Oct 27, 2017

View reviewed changes

dwijnand mentioned this pull request Oct 27, 2017

Disable scope delegation in act #3679

Closed

laughedelic added 2 commits October 27, 2017 13:32

Made test checks less specific

4bba70b

Switched test and compile clean tests

f2aab78

To check that test:compile::clean deletes only test-generated classes

dwijnand reviewed Oct 27, 2017

View reviewed changes

Returned back cleanFilesTask without args

f486d9d

laughedelic added 2 commits October 28, 2017 21:25

Added cleanKeepFiles to the compile task scope

243d028

Minor fix in the cleanKeepFiles key description

453cdb1

Duhemm added the Spree label Oct 30, 2017

dwijnand modified the milestones: 1.1.0, 1.something Nov 1, 2017

dwijnand removed this from the 1.something milestone Nov 29, 2017

laughedelic mentioned this pull request Feb 25, 2018

Feature Request: test:clean #2825

Closed

laughedelic mentioned this pull request Apr 11, 2018

Collect results of Lambda World's spree scalacenter/sprees#40

Closed

eed3si9n requested changes May 30, 2018

View reviewed changes

eed3si9n changed the base branch from 1.x to develop August 31, 2018 04:59

eed3si9n added the x/waffle label Sep 18, 2018

eatkins mentioned this pull request Feb 3, 2019

Glob performance #4539

Merged

1 task

eed3si9n closed this Mar 22, 2019

eed3si9n removed the ready label Mar 22, 2019

Conversation

laughedelic commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvican left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvican Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvican Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laughedelic commented Oct 28, 2017

Uh oh!

dwijnand commented Oct 30, 2017

Uh oh!

jvican commented Oct 30, 2017

Uh oh!

laughedelic commented Nov 1, 2017

Uh oh!

eed3si9n left a comment

Choose a reason for hiding this comment

Uh oh!

jvican commented May 31, 2018

Uh oh!

eed3si9n commented Jun 1, 2018

Uh oh!

dwijnand commented Jun 1, 2018

Uh oh!

laughedelic commented Oct 27, 2017 •

edited

Loading

jvican Oct 27, 2017 •

edited

Loading

jvican Oct 27, 2017 •

edited

Loading