Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Fixes #227; add mill clean #315

Merged
merged 8 commits into from
May 16, 2018
Merged

[WIP] Fixes #227; add mill clean #315

merged 8 commits into from
May 16, 2018

Conversation

guilgaly
Copy link
Contributor

@guilgaly guilgaly commented May 8, 2018

See issue #227. I'm picking up after pull-request #253 (closed due to inactivity).

This seems to work for simple use cases (mill clean, mill myModule someOtherModule, mill myModule.myTask...). Some limitations:

  • it won't show an error if you try to execute clean on a module/task which does not exist...
  • it won't work with, say, mill _.myTask...

Question: To improve that, I guess we'd need to resolve the target similarly to mill.main.RunScript#resolveTasks, except able to resolve either a module (not the module's default task) or a task? Would that make sense to do?

TODO:

  • Use a similar resolution mechanism as mill resolve
  • Improve the "clean all" case
  • Test with cross modules too
  • Update docs
  • See if we can avoid special-casing mill clean and handle it as mill clean __

@lihaoyi
Copy link
Member

lihaoyi commented May 8, 2018

Resolving modules/tasks the same way mill resolve works seems like a reasonable thing to do.

@guilgaly
Copy link
Contributor Author

guilgaly commented May 8, 2018

I'll look into doing just that, probably this weekend. 👍


val pathsToRemove =
if (targets.isEmpty)
Right(List(rootDir))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case mill exits with a non-zero exit code, presumably because it fails to delete out/clean, so that's a bit ugly... We should probably list the content of out/ and filter out out/clean.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get output of clean command with T.ctx().dest, and filtering out should be simple in this case


val pathsToRemove =
if (targets.isEmpty)
Right(ammonite.ops.ls(rootDir).filterNot(keepPath))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out the error on "filter out" was caused by deleting out/mill-worker-1. For now I have hard-coded a filter for anything like out/mill-worker-N, but:

  • I'm not sure this is what we'd want?
  • If we do, I don't know if there is a better way to filter those folders ?

@Ammonite-Bot
Copy link
Collaborator

Ammonite-Bot commented May 12, 2018 via email

@guilgaly guilgaly changed the title [WIP] fixes #227; add mill clean Fixes #227; add mill clean May 12, 2018
@lihaoyi
Copy link
Member

lihaoyi commented May 14, 2018

Looks ok to me. @rockjam want to take a look? If you're happy we can merge this

def clean(evaluator: Evaluator[Any], targets: String*) = mill.T.command {
val rootDir = ammonite.ops.pwd / OutDir

val KeepPattern = "(mill-.+)".r.anchored
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an idea: what if instead of this regex, we simply treated mill clean as the same as mill clean __? That should correctly delete everything that needs to be deleted while leaving the not-to-be-deleted files untouched

Copy link
Contributor Author

@guilgaly guilgaly May 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just beat me to it! I was starting to think along those line, mainly for better consistency with the other tasks in the main module, but you're right that it should also remove the special-case code with the somewhat arbitrary regex. 👌

I'll try to implement that and see if there is any issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilgaly do you want to try out this one in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rockjam I'd say this PR is already useful, so I'll rather try to write the improved version for another future PR.

@lihaoyi & @rockjam : one thing I'm wondering about is wether we have a way to list all available external modules (like the GenIdea one)? As we'll want to clean their outputs too when doing mill clean __.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have a way to list all of them right now; they just live arbitrarily on the classpath, and JVM classpath-scanning is not well supported (slow, unreliable, ...). Currently, their target directories are also randomly scattered throughout the out/ folder.

Perhaps we could just put all the ExternalModules into an mill-external folder, and add that folder to the hardcoded list of things to rm?

@guilgaly guilgaly changed the title Fixes #227; add mill clean [WIP] Fixes #227; add mill clean May 14, 2018
@rockjam
Copy link
Collaborator

rockjam commented May 14, 2018

@guilgaly @lihaoyi looks ok to me too.

@rockjam rockjam merged commit 730a40d into com-lihaoyi:master May 16, 2018
@guilgaly guilgaly deleted the mill-clean branch May 16, 2018 21:00
@lefou lefou added this to the 0.2.1 milestone May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants