cabal new-clean #5357

typedrat · 2018-06-01T18:41:23Z

I think it needs some clean-up most likely, but by my understanding of what it's supposed to do, it's at least feature complete.

Please include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.
If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

Fixes #2957

dcoutts

Looks ok, but I think that because we extend the install command, this will give the new-clean command a bazillion flags that are effectively unused and so unnecessary.

Have a look at the project orchestration module and see if we can get ahold of a dist dir layout without needing much more than the GlobalFlags.

dcoutts · 2018-06-01T21:51:49Z

cabal-install/Distribution/Client/CmdClean.hs

+          ( removeDirectoryRecursive )
+
+cleanCommand :: CommandUI (ConfigFlags, ConfigExFlags, InstallFlags, HaddockFlags)
+cleanCommand = Client.installCommand


Why is it derived from the install command? Do we need so many flags?

dcoutts · 2018-06-01T21:53:41Z

cabal-install/Distribution/Client/CmdClean.hs

+
+    let distDir = distDirectory (distDirLayout baseCtx)
+
+    removeDirectoryRecursive distDir


Ahh I see, so using the ProjectOrchestration it looks like we need all this info just to establish the dist dir layout. I strongly suspect however that we could construct the dist dir layout from a whole lot less, and then we wouldn't need to take all these various sets of flags, and would not need to derive from the install command UI.

typedrat · 2018-06-01T22:44:21Z

@dcoutts I know this was covered in IRC but just for others/posterity, the reason it's like that is to support removing individual targets, which requires knowing most of the build information.

typedrat · 2018-06-06T08:22:11Z

Huh, was when exported from somewhere else but only in 7.10.3? I'm trying to figure out why that CI error is showing up.

typedrat · 2018-06-08T20:45:12Z

Do we want to keep the wildly argumentative new-clean that allows selective cleaning (and refine that feature so it works better than it does now) or do we give up on that functionality to be able to simplify the flags it will take?

Even dropping the functionality, there are a few things that we can't get around being able to know (unless we want to only support default use): dist dir prefix and project file name. That's not that bad, though.

typedrat · 2018-06-08T21:23:34Z

I like that a lot better, and personally I don't know if the trade-off in complexity is worth it. If it's going to know how to map a target into the information it needs to clean things up, it'll need to know enough to build stuff.

Made changes in question, no longer uses the complex UI.

typedrat · 2018-06-09T21:47:19Z

It is now properly documented, and unless we really want target-specific cleaning enough to add the substantial complexity increase, I believe this just needs squashing and merging.

23Skidoo · 2018-06-10T10:58:33Z

I agree that granular clean is not required for 3.0, for which our main goal is feature parity.

23Skidoo

Code LGTM, but I'd like the documentation to explicitly list the supported flags, especially --save-config.

23Skidoo · 2018-06-10T11:00:56Z

Cabal/doc/nix-local-build.rst

+
+``cabal new-clean [FLAGS]`` cleans up the temporary files and build artifacts
+stored in the ``dist-newstyle`` folder. By default, it removes the entire folder,
+but it can also selectively remove the build artifacts and spare the caches within.


I guess this is no longer true? Or does this mean --save-config? In any case, the wording is a bit vague.

Also please document the distPref and projectFile flags here.

That does mean --save-config.

Those flags aren't documented for any of the other commands that take them, they're exactly the same as the ones of the same meaning taken by every other new- command so far, just replicated so I don't drag in every flag down to the municipal flag of Kansas City to get the two things I need.

23Skidoo · 2018-06-10T11:02:29Z

cabal-install/changelog

@@ -1,6 +1,9 @@
 -*-change-log-*-

 2.4.0.0 (current development version)
+	* Completed the 'new-clean' command (#5357). By default it removes
+	the entire 'dist-newstyle' directory, but can also selectively remove


but can also selectively remove only build artifacts.

Seems like this bit also needs updating/rewording.

quasicomputational · 2018-06-10T11:51:39Z

cabal-install/Distribution/Client/CmdClean.hs

+            cleanProjectFile (\pf flags -> flags {cleanProjectFile = pf})
+            (reqArg "FILE" (succeedReadE Flag) flagToList)
+        , option ['s'] ["save-configure"]
+            "Save configuration, only remove build artefacts"


s/artefacts/artifacts/

quasicomputational · 2018-06-10T11:54:20Z

cabal-install/Distribution/Client/CmdClean.hs

+  }
+
+cleanAction :: CleanFlags -> [String] -> GlobalFlags -> IO ()
+cleanAction CleanFlags{..} _ _ = do


Maybe fail on unexpected arguments, so that, if we want to extend new-clean, we won't be breaking any code mistakenly relying on being able to pass junk on the command line.

You're right, that's pretty important/basic.

quasicomputational · 2018-06-11T08:32:08Z

Oh, changelog note? Actually I'll go ahead and write one up myself.

typedrat added the cabal-install: nix-local-build label Jun 1, 2018

typedrat self-assigned this Jun 1, 2018

typedrat requested review from 23Skidoo and hvr June 1, 2018 18:41

typedrat added this to InProgress in Have new-build become build (GSOC2018) Jun 1, 2018

dcoutts previously requested changes Jun 1, 2018

View reviewed changes

typedrat moved this from In Progress to Done in Have new-build become build (GSOC2018) Jun 9, 2018

23Skidoo approved these changes Jun 10, 2018

View reviewed changes

quasicomputational reviewed Jun 10, 2018

View reviewed changes

Add cabal new-clean (closes haskell#2957)

e931db4

typedrat force-pushed the new-clean branch from 2efeee5 to e931db4 Compare June 11, 2018 00:06

typedrat merged commit e5f8dc3 into haskell:master Jun 11, 2018

typedrat changed the title ~~[WIP!] cabal new-clean~~ cabal new-clean Jun 13, 2018

cabal new-clean #5357

cabal new-clean #5357

typedrat commented Jun 1, 2018 •

edited

dcoutts left a comment

dcoutts Jun 1, 2018

dcoutts Jun 1, 2018

typedrat commented Jun 1, 2018

typedrat commented Jun 6, 2018

typedrat commented Jun 8, 2018 •

edited

typedrat commented Jun 8, 2018

typedrat commented Jun 9, 2018

23Skidoo commented Jun 10, 2018

23Skidoo left a comment

23Skidoo Jun 10, 2018

23Skidoo Jun 10, 2018

typedrat Jun 10, 2018

23Skidoo Jun 10, 2018

quasicomputational Jun 10, 2018

quasicomputational Jun 10, 2018

typedrat Jun 10, 2018

quasicomputational commented Jun 11, 2018


		let distDir = distDirectory (distDirLayout baseCtx)

		removeDirectoryRecursive distDir

cabal new-clean #5357

cabal new-clean #5357

Conversation

typedrat commented Jun 1, 2018 • edited

dcoutts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typedrat commented Jun 1, 2018

typedrat commented Jun 6, 2018

typedrat commented Jun 8, 2018 • edited

typedrat commented Jun 8, 2018

typedrat commented Jun 9, 2018

23Skidoo commented Jun 10, 2018

23Skidoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quasicomputational commented Jun 11, 2018

typedrat commented Jun 1, 2018 •

edited

typedrat commented Jun 8, 2018 •

edited