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

cabal new-clean #3835

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@RyanGlScott
Member

RyanGlScott commented Sep 13, 2016

This is my first major contribution to cabal-install, so I'm probably doing this all wrong. Please scrutinize heavily :)

The code for new-clean's cleanAction is mostly a direct copy from that of clean, but in addition, it also removes dist-newstyle and cabal.project.local, if they exist.

I'm not sure if the flags for clean all make sense for new-clean. For instance, clean has a --save-configure flag to allow the user to avoid removing the dist/setup-config file. Does this make sense for new-clean?

Fixes #2957.

@23Skidoo

This comment has been minimized.

Member

23Skidoo commented Sep 13, 2016

/cc @dcoutts

commandSynopsis = "Cleans up after a Nix-local build.",
commandUsage = usageAlternatives "new-clean" [ "[FLAGS]" ],
commandDescription = Just $ \_ ->
"Cleans up Nix-local build files such as .hi, .o, preprocessed sources, etc.",

This comment has been minimized.

@23Skidoo

23Skidoo Sep 13, 2016

Member

Since you already mentioned nix-local-build in the synopsis, I think you can just say "intermediate build files".

cleanDistPref (\d flags -> flags { cleanDistPref = d })
showOrParseArgs
,option "s" ["save-configure"]
"Do not remove the configuration file (dist/setup-config) during cleaning. Saves need to reconfigure."

This comment has been minimized.

@23Skidoo

23Skidoo Sep 13, 2016

Member

Whitespace here looks gratuitous.

@23Skidoo

This comment has been minimized.

Member

23Skidoo commented Sep 13, 2016

I think that in the case of build-type: Custom you also need to run setup clean, because otherwise clean hooks won't be run. See also https://github.com/haskell/cabal/blob/master/cabal-install/Distribution/Client/SetupWrapper.hs#L213.

Related: #3600.

@RyanGlScott

This comment has been minimized.

Member

RyanGlScott commented Sep 13, 2016

I think that in the case of build-type: Custom you also need to run setup clean, because otherwise clean hooks won't be run. See also https://github.com/haskell/cabal/blob/master/cabal-install/Distribution/Client/SetupWrapper.hs#L213.

Sorry, I don't understand this comment. Are you saying that I need to add hooks for new-clean? I couldn't find any hooks for the other new-* commands, so I didn't bother adding them for new-clean.

Or are you saying I need to change the way the Setup script works?

@ezyang

This comment has been minimized.

Contributor

ezyang commented Sep 13, 2016

I'm not sure if the flags for clean all make sense for new-clean. For instance, clean has a --save-configure flag.

Maybe. That would be something like, running ./Setup clean --save-configure for every package in the project, but not actually deleting anything. Honestly I'm not sure the complexity is buying us all that much.

Sorry, I don't understand this comment. Are you saying that I need to add hooks for new-clean? I couldn't find any hooks for the other new-* commands, so I didn't bother adding them for new-clean.

Nope. The point is that some Custom setup scripts may drop build files in NOT dist-newstyle. Like a configure script might stick a header file somewhere random. A Custom clean would be able to remove the file, even if deleting dist isn't enough.

FWIW, I think custom clean is a mistake and if you want something cleaned put it in the dist directory. And it's a pain in the ass because you end up building the Setup script, JUST to run clean. That's no fun.

let flags' = flags { cleanDistPref = toFlag distPref }
noExtraFlags args

pdfile <- defaultPackageDesc verbosity

This comment has been minimized.

@ezyang

ezyang Sep 13, 2016

Contributor

This looks wrong. There's no reason to believe you actually have a specific package if you new-clean. Say you're in the top-level directory of a project. What is pdfile going to be here? Bogus.

Meta-commentary, I probably would NOT go about implementing this as a copy paste from the existing clean. Better to just do it from scratch, I think.

@ezyang

This comment has been minimized.

Contributor

ezyang commented Sep 13, 2016

Needs a test.

@ezyang ezyang self-assigned this Sep 13, 2016

@ezyang ezyang added this to the 2.0 milestone Sep 13, 2016

@ezyang ezyang assigned RyanGlScott and unassigned ezyang Sep 13, 2016

@ezyang

This comment has been minimized.

Contributor

ezyang commented Sep 13, 2016

Thanks for the patch! Definitely filling a missing hole in the implementation.

@dcoutts

This comment has been minimized.

Member

dcoutts commented Sep 13, 2016

@RyanGlScott thanks for the contribution!

I agree with @ezyang that we probably want to approach it from afresh. One significant change with the new-* commands is that they mostly work at the level of a whole project, and only sometimes at individual package level.

So I think for new-clean the main thing we want to do is clean the whole project, which means rm -r'ing the dist-newstyle directory. That gets us 90% of the way there. Then there are some other bits and bobs. Package .cabal files can specify extra files to clean, and so those should be removed too.

The equivalent of -s / --save-configure would be to keep or not keep the cabal.project.local file.

Whether or not we should run the "clean" hook of custom Setup scripts is a bit of an open question. It's rather annoying to have to compile a Setup script just so we can run clean, which 99% of the time does nothing more than rm the dist dir. What if we cannot configure the project or build the Setup scripts, does cleaning fail? I could be persuaded that clean being part of custom Setup is just a design flaw, and all generated files should live in dist except for those few listed in the extra clean files in the .cabal file.

So I'd focus on whole-project clean first. If we really want to later we could think about adding "clean, but only this bit, not everything", e.g `cabal clean ./" to mean this target (in the same way we have build targets). But that's a bonus not a necessity.

@ezyang

Don't read in package description to new-clean; it's a project-wide concept.

@RyanGlScott RyanGlScott closed this Jan 4, 2017

@RyanGlScott

This comment has been minimized.

Member

RyanGlScott commented Jan 4, 2017

Oops, I was doing a rebase and accidentally closed this :\

I'll re-open when I've implemented the suggestions above.

@RyanGlScott

This comment has been minimized.

Member

RyanGlScott commented Feb 25, 2017

...or not. Sadly, the implementation has become substantially more complicated than when I looked at this last, and with my limited knowledge of the Cabal API, I doubt that I'm going to have the ability to make this happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment