To prevent download ghc when using `stack clean`. by waddlaw · Pull Request #4268 · commercialhaskell/stack

waddlaw · 2018-08-27T09:10:35Z

stack clean always download the ghc even if it already deleted.

$ stack --version
Version 1.10.0, Git revision 65dd1d673a75d867325bb1c70f5170386900dd80 (dirty) (6415 commits) x86_64 hpack-0.29.6

$ stack clean
Preparing to install GHC to an isolated location.
This will not interfere with any system-level installation.
Preparing to download ghc-7.10.2 ...^C

Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.

Please include the following checklist in your PR:

Any changes that could be relevant to users have been recorded in the ChangeLog.md
The documentation has been updated, if necessary.

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

dbaynard · 2018-08-27T19:09:02Z

Thanks, @waddlaw — this relates to #4181, correct?

I'm going to review, now.

dbaynard

Thanks again.

There are 2 major issues which must be resolved.

The check in the stack code base that there is a valid GHC is now made optional, but downstream code has not been changed to reflect this. This makes that code more fragile. I can't speak for everyone on the project, though, so I'm getting another opinion.
This PR duplicates certain code paths due to bugs which have been (mostly) resolved. I've pinged @mgsloan who should be able to confirm this. Most of the additions to the code here fall into this category.

dbaynard · 2018-08-27T19:12:08Z

@@ -631,8 +631,8 @@ cleanCmd opts go =
  -- See issues #2010 and #3468 for why "stack clean --full" is not used
  -- within docker.


So interestingly, both #2010 and #3468 have been closed — but #3471 is still open.

@mgsloan can this be simplified, now?

dbaynard · 2018-08-27T19:14:10Z


 withBuildConfigExt
    :: Bool
+    -> Bool -- ^ If perform the stack clean


I'm concerned by the boolean blindness, here. The following types would be helpful.

data WithDocker = SkipDocker | WithDocker data IsClean = NotClean | IsClean

But see my comments on whether the docker checks are still necessary.

Agreed, making another type that's iso to Bool is usually better.

To me the naming seems too purpose specific. I think it makes sense to instead have the name correspond to the behavioral change. That, is the name should specify whether or not it requires ghc to be installed.

dbaynard · 2018-08-27T19:16:30Z

-    withBuildConfigExt False go Nothing inner Nothing
+    withBuildConfigExt False False go Nothing inner Nothing

 -- | See issue #2010 for why this exists. Currently just used for the


See comment below

dbaynard · 2018-08-27T19:16:40Z

+    withBuildConfigExt False True go Nothing inner Nothing
+
+-- | See issue #2010 for why this exists. Currently just used for the
+-- specific case of "stack clean --full".


See comment below

dbaynard · 2018-08-27T19:25:20Z

            }

-    (mghcBin, compilerBuild, _) <- ensureCompiler sopts
+    (mghcBin, compilerBuild, _) <- if bcClean bconfig then pure (Nothing, CompilerBuildStandard, False) else ensureCompiler sopts


This change is making the presence of a compiler optional, and so the code should reflect this — yet this implementation does not.

Subsequent code uses the compilerBuild option; it is faked here, making that code more fragile. It might be better that it has type Maybe CompilerBuild here; in this way the possible absence of a compiler is reflected in the type system. This then propagates to EnvConfig which I believe will need changes.

mgsloan · 2018-08-27T21:40:24Z

@dbaynard I don't recall the details of this part of the code, so it's best just to look at what it's doing. I agree about avoiding use of Bool. I left a comment about naming considerations - I imagine that skipping compiler installation is not unique to the clean command.

waddlaw · 2018-08-31T09:00:20Z

@dbaynard Thank you for reviewed!

this relates to #4181, correct?

I don't know that directly related to it. But, bypassed the ghc download at least.

There are 2 major issues which must be resolved.

OK. I will try to fix it.

I left a comment about naming considerations - I imagine that skipping compiler installation is not unique to the clean command.

That makes sense.

waddlaw · 2018-09-02T06:32:16Z

@dbaynard Are you sure about this?

mihaimaruseac · 2018-10-04T00:08:41Z

Ping on this, as it's becoming stale.

dbaynard · 2018-11-07T17:21:21Z

Ah, thank you for bearing with me — I must have looked at this 3 or 4 times and didn't notice the new commits.

I'll check shortly, and then it may be worth looking at the possibilities for #4390.

dbaynard

OK, I'm going to try to implement an integration test, then merge.

dbaynard · 2018-11-16T12:45:57Z

    -- 'wantedCompilerL', which provides the version specified by the
    -- build plan.
-    ,envConfigCompilerBuild :: !CompilerBuild
+    ,envConfigCompilerBuild :: !(Maybe CompilerBuild)


Suggested change

,envConfigCompilerBuild :: !(Maybe CompilerBuild)

,envConfigCompilerBuild :: !(Maybe !CompilerBuild)

I think we want to force the whole thing.

waddlaw · 2018-11-17T01:35:02Z

@dbaynard thanks!

dbaynard · 2018-11-17T15:38:46Z

@waddlaw have you seen #4390? Would you like to try to implement that?

waddlaw · 2018-11-17T16:39:16Z

I’m more than happy to help you :) I'll do that.

dbaynard · 2018-11-17T19:13:54Z

Great!

qrilka · 2018-11-21T07:38:18Z

I have note about this PR - SkipDownloadCompiler doesn't quite reflect what's going on when this value get passed: it turns off any compiler checks. I'm not sure what could be a more suitable name here but it's better to have something more explicit and probably some haddocks could be added

dbaynard · 2018-11-21T22:19:53Z

Yes — any suggestions welcome. In particular, there is #4405 which builds on it. I'll note your comment in my review. So SkipCheckCompiler would be a more precise name?

qrilka · 2018-11-22T06:10:24Z

Thanks, sounds good to me though word order is confusing a bit, I think SkipCompilerCheck reads better, what do you think?

waddlaw · 2018-11-23T01:38:43Z

Thanks @qrilka
It naming so nice. Would you like me to contain this change into #4405? or submit another pr?

qrilka · 2018-11-23T05:40:18Z

@waddlaw I think separate commit in #4405 with this change will be OK

qrilka · 2019-01-05T13:49:41Z

@dbaynard it looks like #4480 uncovered problem with this PR and I'm not yet clear how to resolve it: by default stack clean does shallow cleanup deleting dist directories for project packages but to get proper dist directories Stack consults ghc-pkg to get Cabal version and without GHC installed (or not put into the current $PATH) it fails.
I think we should be able to go around that if we could use global packages retrieved from https://github.com/commercialhaskell/stackage-content/blob/master/stack/global-hints.yaml but that changes Stack logic a bit. Do we have any guarantees that global-pkg-db won't contain other Cabal version?
BTW just today I have a Twitter discussion about using arbitrary Cabal with earlier Stackage LTSes - https://twitter.com/qrilka/status/1081538344214020096

dbaynard · 2019-01-06T23:27:37Z

I found the lack of documentation on this precise point really disappointing — I had to go digging through the code and run some tests myself in order to see what stack clean actually did.

Presumably, stack clean ought to remove the directory corresponding to the compiler's Cabal version (that's how it has been implemented). It then should not remove directories corresponding to other 'Cabal' versions. Is that correct?

All this information ought to be in the stack configuration — presumably if somebody has overridden the cabal version, that override would be there? In which case we can use global-hints.yaml. Otherwise we may have to choose between downloading GHC and deleting the wrong dist directory.

As an aside: the integration test has a false positive, related to #4468, which I'm yet to fix.

qrilka · 2019-01-07T08:08:18Z

You're mostly correct in your analysis. There's only 1 thing to note: currently Cabal version is more or less fixed for an LTS (the one shipped with GHC gets used) and Veronika (from the Twitter discussion I linked above) started a fresh issue #4488 to discuss ways around that limitation of Stack.

dbaynard · 2019-01-07T09:29:41Z

Right so if that changes our assumption about global-hints.yaml has to change?

I'll comment on the relevant issues, then. I'm quite confused by the various interactions with stack and Cabal — may seek some clarifications there, too.

But for now, given stack path --dist-dir (which picks the relevant Cabal library) requires ghc-pkg I'll start digging around there.

qrilka · 2019-01-11T11:28:46Z

Somehow missed this message @dbaynard but make it maybe a bit more clear - I don't think we could change our assumptions about global-hints.yaml - that file says which libraries come with a specific GHC version and we use that to get global libraries which go into the smGlobal field of SourceMap type. As for Cabal version - it could be overridden and if we'll try to address concerns in #4488 we'll use that new Cabal version instead of a "global" one which could come either from ghc-pkg results or global-hints.yaml. Does this make sense?

dbaynard requested changes Aug 27, 2018

View reviewed changes

dbaynard requested a review from mgsloan August 27, 2018 19:40

mihaimaruseac added awaiting-response work-in-progress needs changes labels Oct 4, 2018

dbaynard mentioned this pull request Nov 7, 2018

Simple commands should work without ghc installed #4390

Closed

dbaynard self-assigned this Nov 7, 2018

dbaynard added awaiting review and removed awaiting-response labels Nov 7, 2018

dbaynard approved these changes Nov 16, 2018

View reviewed changes

waddlaw and others added 10 commits November 16, 2018 12:55

Add bcClean flag to BuildConfig

1f51ff7

Add extra flag

1f2b95d

Update cleanCmd

8eaed53

Avoid to download ghc

1974a24

Add util functions (and a bit tweak)

391d2e5

Update Changelog

2d8998e

Use WithDocker instead of Bool

f4eb713

Change Bool to proper type

12f0c43

Fix fake value

67c07b2

Add integration test

ae16993

dbaynard merged commit b139364 into commercialhaskell:master Nov 16, 2018

waddlaw deleted the fix-clean branch November 17, 2018 01:35

dbaynard mentioned this pull request Dec 27, 2018

Make Stack's integration tests not depend on $PATH #4468

Open

qrilka mentioned this pull request Jan 4, 2019

stack clean fails due to missing ghc-pkg (on latest from master) #4480

Closed

dbaynard added component: Cabal library and removed awaiting review needs changes work-in-progress component: Cabal library labels Jan 7, 2019

		@@ -631,8 +631,8 @@ cleanCmd opts go =
		-- See issues #2010 and #3468 for why "stack clean --full" is not used
		-- within docker.

	,envConfigCompilerBuild :: !(Maybe CompilerBuild)
	,envConfigCompilerBuild :: !(Maybe !CompilerBuild)

Conversation

waddlaw commented Aug 27, 2018

Uh oh!

dbaynard commented Aug 27, 2018

Uh oh!

dbaynard left a comment

Choose a reason for hiding this comment

Uh oh!

dbaynard Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

dbaynard Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

mgsloan Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

dbaynard Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

dbaynard Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

dbaynard Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

mgsloan commented Aug 27, 2018

Uh oh!

waddlaw commented Aug 31, 2018

Uh oh!

waddlaw commented Sep 2, 2018

Uh oh!

mihaimaruseac commented Oct 4, 2018

Uh oh!

dbaynard commented Nov 7, 2018

Uh oh!

dbaynard left a comment

Choose a reason for hiding this comment

Uh oh!

dbaynard Nov 16, 2018

Choose a reason for hiding this comment

Uh oh!

waddlaw commented Nov 17, 2018

Uh oh!

dbaynard commented Nov 17, 2018

Uh oh!

waddlaw commented Nov 17, 2018

Uh oh!

dbaynard commented Nov 17, 2018

Uh oh!

qrilka commented Nov 21, 2018

Uh oh!

dbaynard commented Nov 21, 2018

Uh oh!

qrilka commented Nov 22, 2018

Uh oh!

waddlaw commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qrilka commented Nov 23, 2018

Uh oh!

qrilka commented Jan 5, 2019

Uh oh!

dbaynard commented Jan 6, 2019

Uh oh!

qrilka commented Jan 7, 2019

Uh oh!

dbaynard commented Jan 7, 2019

Uh oh!

qrilka commented Jan 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

waddlaw commented Nov 23, 2018 •

edited

Loading