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

Make dot and ls dependencies work without ghc installed #4405

Closed
wants to merge 7 commits into from
Closed

Make dot and ls dependencies work without ghc installed #4405

wants to merge 7 commits into from

Conversation

waddlaw
Copy link
Contributor

@waddlaw waddlaw commented Nov 18, 2018

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 fixed #4390. Could you please confirm it?

ChangeLog.md Outdated Show resolved Hide resolved
. updateOpts True (globalOptsL.configMonoidInstallGHCL) (Just False)

-- helper function for update some option
updateOpts :: Bool -> Lens' opts v -> v -> opts -> opts
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this combinator 👍

updateGlobalOpts
= updateOpts (dotTestTargets opts) (globalOptsBuildOptsMonoidL.buildOptsMonoidTestsL) (Just True)
. updateOpts (dotBenchTargets opts) (globalOptsBuildOptsMonoidL.buildOptsMonoidBenchmarksL) (Just True)
. updateOpts True (globalOptsL.configMonoidSkipGHCCheckL) (Just True)
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like the updateOpts is really useful here, how about just set (globalOptsL...) (Just True)? I think it would be clearer that this is always being set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right!

snoyberg and others added 2 commits November 19, 2018 18:31
Co-Authored-By: waddlaw <waddlaw@users.noreply.github.com>
@waddlaw
Copy link
Contributor Author

waddlaw commented Nov 19, 2018

Thank you for the review. updated!

@dbaynard
Copy link
Contributor

Great job, @waddlaw! I'll review shortly. I thought I might have a little longer to find you the next issue to tackle 🤩

Copy link
Contributor

@dbaynard dbaynard left a comment

Choose a reason for hiding this comment

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

Generally good — just a few minor changes required.

It would be good to add some tests, too, perhaps modifying the integration test that checks this for stack clean from the fix to #4181 (although that test needs a little work, too).

@@ -206,6 +206,17 @@ instance Show SetupException where
show UnsupportedSetupConfiguration =
"I don't know how to install GHC on your system configuration, please install manually"

checkDownloadCompiler :: (HasConfig env, HasGHCVariant env)
Copy link
Contributor

Choose a reason for hiding this comment

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

As @qrilka noted, this could do with some documentation. We may want to change the WithDownloadCompiler enum as a result.

@@ -97,6 +97,7 @@ Bug fixes:
[#4314](https://github.com/commercialhaskell/stack/pull/4314)
* Add `--cabal-files` flag to `stack ide targets` command.
* Don't download ghc when using `stack clean`.
* `dot` and `ls dependencies` commands no longer require GHC to be installed. Also, ensures the `--no-install-ghc` flag is respected. See: [#4390](https://github.com/commercialhaskell/stack/issues/4390)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be better as two lines; respecting --no-install-ghc in bug fixes and the lack of GHC requirement merged with the stack clean line above, and moved to Other enhancements.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that 2nd second sentence is misleading, the flag was respected at least in version 1.7.1 already:

qrilka@qdesktop ~/ws/h/stack-test-ghc-7.8.4 $ cat stack.yaml 
resolver: ghc-7.8.4
qrilka@qdesktop ~/ws/h/stack-test-ghc-7.8.4 $ stack --no-install-ghc clean
No compiler found, expected minor version match with ghc-7.8.4 (x86_64-tinfo6) (based on resolver setting in /home/qrilka/ws/h/stack-test-ghc-7.8.4/stack.yaml).
To install the correct GHC into /home/qrilka/.stack/programs/x86_64-linux/, try running "stack setup" or use the "--install-ghc" flag.
qrilka@qdesktop ~/ws/h/stack-test-ghc-7.8.4 $ stack --numeric-version
1.7.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's perfectly clear, thanks!!

. set (globalOptsL.configMonoidSkipGHCCheckL) (Just True)
. set (globalOptsL.configMonoidInstallGHCL) (Just False)

-- helper function for update some option
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- helper function for update some option
-- helper function to update an option

It would be good to document what the Bool represents.

@waddlaw
Copy link
Contributor Author

waddlaw commented Nov 23, 2018

@dbaynard I was able to do it.--system-ghc flag is needed that resolve this issue.

$ stack --numeric-version
1.9.1

$ stack --no-install-ghc --skip-ghc-check --system-ghc dot --external
strict digraph deps {
"clean" [style=dashed];
{rank=max; "rts" [shape=box]; };
"base" -> "ghc-prim";
"base" -> "integer-gmp";
"base" -> "rts";
"clean" -> "base";
"ghc-prim" -> "rts";
"integer-gmp" -> "ghc-prim";
}

$ stack --no-install-ghc --skip-ghc-check dot --external
No compiler found...

msystem is Nothing if missing --system-ghc.

stack/src/Stack/Setup.hs

Lines 386 to 391 in 0b68efc

msystem <-
if soptsUseSystem sopts
then do
logDebug "Getting system compiler version"
getSystemCompiler wc
else return Nothing

stack/src/Stack/Setup.hs

Lines 395 to 399 in 0b68efc

let canUseCompiler compilerVersion arch
| soptsSkipGhcCheck sopts = True
| otherwise = isWanted compilerVersion && arch == expectedArch
isWanted = isWantedCompiler (soptsCompilerCheck sopts) (soptsWantedCompiler sopts)
needLocal = not (any (uncurry canUseCompiler) msystem)

In other words, needLocal = not (any (uncurry canUseCompiler) Nothing).

ghci> :t any
any :: Foldable t => (a -> Bool) -> t a -> Bool

ghci> any (const True) Nothing
False

I think that we should just only enable these option in dot (and ls), or add new --skip-ensure-compiler flag. (This flag is like completely skip ensureCompiler function)

What do you think?

@qrilka
Copy link
Contributor

qrilka commented Nov 23, 2018

I don't think enabling --system-ghc is a way to go here - it could be of a different and maybe incompatible version or there could be no system GHC at all. And the last one is the starting use case in #4390
Also I don't think that having a CLI flag --skip-ensure-compiler is a good idea - it's better to have that option internal-only and disable it only for a selected number of commands which should not fail without proper GHC available

@waddlaw
Copy link
Contributor Author

waddlaw commented Nov 23, 2018

Thanks @qrilka. I understand it.

My implemented checkDownloadCompiler is possibly wrong...?(always skipping ensureCompiler if --no-install-ghc specified)

checkDownloadCompiler _     SkipDownloadCompiler = return (Nothing, Nothing, False)
checkDownloadCompiler sopts WithDownloadCompiler
  | installIfMissing = ensureCompiler sopts
  | otherwise = return (Nothing, Nothing, False)
  where
    installIfMissing = soptsInstallIfMissing sopts

Hence, stack --no-install-ghc --skip-ghc-check dot --external works fine.

@qrilka
Copy link
Contributor

qrilka commented Nov 23, 2018

I think it's wrong, yes, disabling GHC installation doesn't mean that compiler check should get skipped. For dot we should be able to go on without a compiler so SkipDownloadCompiler (or SkipCompilerCheck) should be OK for it. At least that's my understanding of these things.

@waddlaw
Copy link
Contributor Author

waddlaw commented Nov 23, 2018

All right. It was my mistake. I will fix asap!

@mihaimaruseac mihaimaruseac changed the title Fix t4390 - dot and ls dependencies work without ghc installed Make dot and ls dependencies work without ghc installed Jan 22, 2019
@dbaynard
Copy link
Contributor

There are some changes in progress (e.g. #4500) which may affect this.

@waddlaw
Copy link
Contributor Author

waddlaw commented Feb 11, 2019

@dbaynard
Sorry for the delay in work.I think I will work on it.
Is it better to work after # 4500 is merged?

@dbaynard
Copy link
Contributor

It should be merged soon — but if you work on it before then just take into account those changes. I think the only changes left for that PR are in the test suite.

@snoyberg
Copy link
Contributor

I'm looking into this right now as part of my work on cleaning up Stack.Runners. I'm confused as to why this is supposed to work at all. stack dot depends on knowing information about dependencies of global packages, which in turn requires having a locally installed GHC. It seems like the only way to make this work is to ignore global packages entirely, which this PR isn't doing.

snoyberg added a commit that referenced this pull request Mar 18, 2019
@qrilka
Copy link
Contributor

qrilka commented Mar 18, 2019

@snoyberg previously we didn't look into global packages and that's why I filed #4324

@snoyberg
Copy link
Contributor

Thanks for clarifying @qrilka. I've opened up #4632, which will supersede this PR.

@snoyberg snoyberg closed this in e34bfe2 Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simple commands should work without ghc installed
4 participants