Skip to content
This repository was archived by the owner on Oct 7, 2020. It is now read-only.

Don't mix stack with cabal #1557

Merged
merged 10 commits into from
Jan 8, 2020
Merged

Don't mix stack with cabal #1557

merged 10 commits into from
Jan 8, 2020

Conversation

hasufell
Copy link
Member

@hasufell hasufell commented Jan 6, 2020

Fixes #1380 and #1556

@fendor fendor requested review from jneira and fendor January 6, 2020 17:47
@jneira
Copy link
Member

jneira commented Jan 6, 2020

Thanks for the pr, but i m afraid it would break some use cases:

  • now stack users can start to use the right version of cabal to work with hie simply running stack install.hs install-cabal
    • I think we should keep that use case so i would not remove installCabalWithStack
  • The script lets users run the script with one tool but install hie with the other: stack install.hs cabal-hie-8.6.5 or cabal v2-run install.hs stack-hie-8.6.5
    • Run the script with stack and then use cabal to install hie needs execCabalWithOriginalPath to use cabal from the user $PATH and no the cached one locally by stack
    • I am not sure what was the reason for use both tools at once and if we could remove it, maybe @power-fungus could help with that
    • If we remove that double tool uses we should remove cabal-* targets for stack and stack-* targets for cabal

@fendor
Copy link
Collaborator

fendor commented Jan 6, 2020

I am not sure what was the reason for use both tools at once and if we could remove it, maybe @power-fungus could help with that

As far as I know, hie is not dependent on a build-tool. Installation of cabal-install is some legacy stuff that we picked up back from the Makefile, iirc.

now stack users can start to use the right version of cabal to work with hie simply running stack install.hs stack-install-cabal

Same as above, I think, that we can use hie without any cabal on the path. Also, inconsistent for cabal users, as they cant do the same for stack.

The script lets users run the script with one tool but install hie with the other: stack install.hs cabal-hie-8.6.5 or cabal v2-run install.hs stack-hie-8.6.5

If users want to use stack scripts but to install hie with cabal, I think they can be bothered to install cabal themselves. Moreover, we are inconsistent, because you can not do the same the other way around. You can not use cabal to install stack, right?

If we remove that double tool uses we should remove cabal-* targets for stack and stack-* targets for cabal

I think that could make sense.

@jneira
Copy link
Member

jneira commented Jan 6, 2020

Well, i think that use both tools is a pretty common scenario although now hie doesnt require to have cabal installed for all projects (you still need it for projects without stack.yaml), so install the right version of cabal could be handy. But i am not strongly against remove it if you think it does not worth the added complexity.

You can not use cabal to install stack, right?

He, fair enough. I guess there are more cases of users using stack that wants to use or needs cabal that the other way around. But if we keep one i would accept a pr to add the other 😉

But the above is only about the possibility of install cabal using stack. I would remove the cabal-* and stack-* targets if there is no good reasons to keep them.

@fendor
Copy link
Collaborator

fendor commented Jan 6, 2020

I think it is a good idea to decouple cabal and stack builds, e.g. a cabal build should not depend on stack in any way.
However, retaining the possibility to install cabal via stack and vice-versa is fine for me.

But the above is only about the possibility of install cabal using stack. I would remove the cabal-* and stack-* targets if there is no good reasons to keep them.

I think that makes sense.

@hasufell
Copy link
Member Author

hasufell commented Jan 6, 2020

IMO, packages should never install toolchain and build tools unquestioned. I don't want to use stack and it breaks HIE installation as you can see in #1556

People using HIE are developers and they should be bothered to install their toolchain and should expect to have proper control over it without packages arbitrarily invoking stack, downloading GHCs that I have already installed and messing with my global state.

I think that makes sense.

I cannot follow. Is there anything actionable for this PR?

Edit: ultimately, when looking at the shakefile, I think it is not useful at all. Almost everything is phony rules, which defeats the entire purpose of shake. It seems this can be easily rewritten with a Makefile or a simple shell script. But this is outside of this PRs scope, which just unbreaks HIE.

@fendor
Copy link
Collaborator

fendor commented Jan 6, 2020

I cannot follow. Is there anything actionable for this PR?

I dont think so, that is something for a follow-up PR.

Edit: ultimately, when looking at the shakefile, I think it is not useful at all. Almost everything is phony rules, which defeats the entire purpose of shake. It seems this can be easily rewritten with a Makefile or a simple shell script. But this is outside of this PRs scope, which just unbreaks HIE.

I agree partially. A Makefile is not platform independent, so no, that would not be a solution. Neither is a shell script. The initial reason was to simplify the installation, which, judging by the amount of issues we receive now and before using this installation script, succeeded.
The shake script was written when installation was more complex. However, using shake is overkill and it could be reduced to a simpler haskell script.

@hasufell
Copy link
Member Author

hasufell commented Jan 6, 2020

A Makefile is not platform independent, so no, that would not be a solution. Neither is a shell script.

Right, I forgot windows users. Shame on me.

@hasufell hasufell requested a review from jneira January 6, 2020 22:54
@jneira
Copy link
Member

jneira commented Jan 6, 2020

Trying to execute the script with stack throws an build error cause cabal-install-parsers is not in stackage. Adding it to install/shake.yaml make stack ask for other packages:

In the dependencies for cabal-install-parsers-0.2:
    Cabal-2.4.1.0 from stack configuration does not match ^>=3.0.0.0  (latest matching version is 3.0.0.0)
    aeson-1.4.5.0 from stack configuration does not match ^>=1.4.6.0  (latest matching version is 1.4.6.0)
    binary-instances must match ^>=1, but the stack configuration has no specified version  (latest matching version is 1)
    lukko must match ^>=0.1.1, but the stack configuration has no specified version  (latest matching version is 0.1.1.1)
needed due to hie-install-0.8.0.0 -> cabal-install-parsers-0.2

Not sure if we can get a package set to build this version with stack. Maybe some cpp would be needed (as anticipated in #1380 (comment))

Ideally the script should be buildable with all resolver of hie stack-*.yaml, to let users use the same ghc building the script and building hie.

@hasufell
Copy link
Member Author

hasufell commented Jan 6, 2020

Trying to execute the script with stack throws an build error

Hm, why did the CIs succeed then? 😕

@fendor
Copy link
Collaborator

fendor commented Jan 6, 2020

@hasufell Currently enabled CI is actually not even building the shake script... There is some CI for azure, but azure is not yet enabled for this project :(

@hasufell
Copy link
Member Author

hasufell commented Jan 6, 2020

I CPPed the relevant bits and also changed the README. I didn't run a full stack build, but at least it starts building.

@jneira
Copy link
Member

jneira commented Jan 7, 2020

@hasufell have you tested it with cabal-2.4.1.0? If it doesnt work with that version we should bump up the cabal required version to 3.0.0.0

@jneira
Copy link
Member

jneira commented Jan 7, 2020

@hasufell thanks for include the remove of obsolete targets, it looks much better now.

As for stack-install-cabal, i would accept remove it only to being able to remove the withoutStackCachedBinaries monstrosity. I hope users will not miss it but we always can restore it.

The docs in https://github.com/haskell/haskell-ide-engine/blob/master/docs/Build.md becomes obsolete with this pr too, but the can be fixed in a follw up pr if you want.

Let me know if you need any help with the last requested changes, i promise they are the last ones 😄

@hasufell
Copy link
Member Author

hasufell commented Jan 7, 2020

As for stack-install-cabal, i would accept remove it only to being able to remove the withoutStackCachedBinaries monstrosity.

That was already done.

@hasufell
Copy link
Member Author

hasufell commented Jan 7, 2020

have you tested it with cabal-2.4.1.0?

Seems to work fine

@hasufell
Copy link
Member Author

hasufell commented Jan 7, 2020

If you have further remarks, I suggest you push your changes to this PR.

@hasufell hasufell requested a review from jneira January 7, 2020 12:41
@lukel97
Copy link
Collaborator

lukel97 commented Jan 7, 2020

I am personally in favour of not having the install script install cabal via stack, which seems kind of sneaky. Can we double check that hie still relies on it?

@fendor
Copy link
Collaborator

fendor commented Jan 7, 2020

@bubba Hie does not rely on it and afaik never really has. I think it was needed to install Cabal to speed up cabal-helper, but no one is even sure that this was ever necessary.

@lukel97
Copy link
Collaborator

lukel97 commented Jan 7, 2020

@fendor probably just best then to let cabal-helper install Cabal? Also seeing as with a hie.yaml file it might not be needed

@fendor
Copy link
Collaborator

fendor commented Jan 7, 2020

I am not sure that ever worked, since cabal-helper installs Cabal into ~/.cache/cabal-helper/Cabal or something like that. And we used cabal v1-install. Currently just cabal-helper takes care of it and I think it is good the way it is.

@lukel97
Copy link
Collaborator

lukel97 commented Jan 7, 2020

I am not sure that ever worked, since cabal-helper installs Cabal into ~/.cache/cabal-helper/Cabal or something like that. And we used cabal v1-install. Currently just cabal-helper takes care of it and I think it is good the way it is.

cabal-helper searches for it installed globally first before installing its own copy, but agreed anyway

@fendor
Copy link
Collaborator

fendor commented Jan 7, 2020

@bubba Ah, did not know that! however, we are not installing Cabal for some time now so, it seems like nobody is complaining right now.

@jneira
Copy link
Member

jneira commented Jan 8, 2020

If you have further remarks, I suggest you push your changes to this PR.

I'll pushed the required changes upt to +85-202 😁
@hasufell thanks a lot for the pr, the script is cleaner now

The help looks now:

Usage:
    stack install.hs <target>
    or
    cabal v2-run install.hs --project-file install/shake.project <target>

Targets:
    help                Show help message including all targets

    hie                 Build hie with the latest available GHC and the data files
    latest              Build hie with the latest available GHC
    data                Get the required data-files for `hie` (Hoogle DB)
    hie-8.4.4           Builds hie for GHC version 8.4.4
    hie-8.6.4           Builds hie for GHC version 8.6.4
    hie-8.6.5           Builds hie for GHC version 8.6.5

    ghcs                Show all GHC versions that can be installed via `cabal-build`.

    icu-macos-fix       Fixes icu related problems in MacOS

I have taken the opportunity to change some target names and make them shorter:

  • build -> hie
  • build-latest -> latest
  • build-data -> data
  • cabal-ghcs -> ghcs

Do you agree with new names @fendor @bubba?
We'll have to change the docs in the vscode extension to match them

@jneira jneira requested a review from fendor January 8, 2020 07:51
@lukel97
Copy link
Collaborator

lukel97 commented Jan 8, 2020

@jneita LGTM

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

It is great that we are simplifying the installation script!

@jneira jneira merged commit e332055 into haskell:master Jan 8, 2020
@jneira jneira mentioned this pull request Jan 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shake install with cabal shouldn't depend on having stack installed
4 participants