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

Specify full version of GHC, not just major version WAS: Stack installs wrong version of GHC #736

Closed
hnsl opened this issue Aug 8, 2015 · 16 comments
Assignees
Milestone

Comments

@hnsl
Copy link

@hnsl hnsl commented Aug 8, 2015

Using resolver nightly-2015-06-16 which is based on ghc-7.10.1 causes stack to install ghc-7.10.2 when I run stack setup.

It's also not possible to specify ghc-7.10.2 as a resolver. I can only specify ghc-7.10 which makes no sense as that's not even a real version of ghc. It doesn't mean anything as version numbers are arbitrary incremented.

@borsboom
Copy link
Contributor

@borsboom borsboom commented Aug 8, 2015

Stack only cares that the correct GHC major version (e.g. 7.10.*) is used for a snapshot, and stack setup will only install the latest for that major version (to be precise, the version specified in stack-setup.yaml). However, if you already have an older GHC 7.10 (e.g. 7.10.1), it should use that (won't force you to upgrade to 7.10.2).

@hnsl
Copy link
Author

@hnsl hnsl commented Aug 8, 2015

Why? The snapshot nightly-2015-06-16 clearly states that it's based on ghc-7.10.1. ghc-7.10.2 contains back compatibility breaking changes.

Related back compatibility breaking change:

https://downloads.haskell.org/~ghc/7.10.1/docs/html/libraries/ghc/TcRnDriver.html

https://downloads.haskell.org/~ghc/7.10.2/docs/html/libraries/ghc-7.10.2/TcRnDriver.html

Related issue: ucsd-progsys/liquidhaskell#420

@borsboom
Copy link
Contributor

@borsboom borsboom commented Aug 8, 2015

I think we'll have to see if @snoyberg can give more detail about the reasoning, but my assumption is because it was thought to be the more desirable behaviour in most cases. And we must have believed "Patchlevels are bug-fix releases only, and never change the programmer interface to any system-supplied code."

I could definitely see supporting a more strict interpretation of GHC versions being desirable, at least optionally, so worth discussing here. In the mean time, you should be able to work around this by installing GHC 7.10.1 yourself and using stack --system-ghc.

@snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Aug 8, 2015

This was by design originally, for exactly the reasons @borsboom mentioned, but I was conflicted about it, and in hindsight think it was the wrong decision. I'm in favor of moving to:

  • Specifying GHC version explicitly
  • Having resolver: ghc-7.10 be a deprecated shortcut for "latest ghc-7.10"
  • Extending the GHC format to include explicit versions

One concern with a change like this is that it won't play nicely with custom GHC builds, so we need to consider how exactly to deal with that situation.

@snoyberg snoyberg added this to the 0.2.0.0 milestone Aug 8, 2015
@snoyberg snoyberg changed the title Stack installs wrong version of GHC Specify full version of GHC, not just major version WAS: Stack installs wrong version of GHC Aug 10, 2015
@mgsloan mgsloan self-assigned this Aug 12, 2015
mgsloan added a commit that referenced this issue Aug 13, 2015
* Requires the exact GHC version specified by the snapshot.

* Deprecates major-version (ghc-7.10, ghc-7.8) resolvers, but still
  supports them.  They resolve to the newest installed / available ghc
  matching that number.

* Changes the format of stack-setup.yaml, and so changes which URL is
  used to find it (in order to not break old stack versions).

* Refactors ensureTool code, as it already had a lot of special cases,
  which I found confusing.  Main cause is that I needed to pass in a
  'CompilerVersion' instead of 'Version', but just for installing ghc,
  not for git.
mgsloan added a commit that referenced this issue Aug 13, 2015
* Requires the exact GHC version specified by the snapshot.

* Deprecates major-version (ghc-7.10, ghc-7.8) resolvers, but still
  supports them.  They resolve to the newest installed / available ghc
  matching that number.

* Changes the format of stack-setup.yaml, and so changes which URL is
  used to find it (in order to not break old stack versions).

* Refactors ensureTool code, as it already had a lot of special cases,
  which I found confusing.  Main cause is that I needed to pass in a
  'CompilerVersion' instead of 'Version', but just for installing ghc,
  not for git.

* Introduces a 'CompilerVersion' type, and changes some naming to
  specify that compiler versions are being passed around rather than ghc
  versions.  The change could be a simpler without this, but this will
  be helpful for GHCJS support.
@mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Aug 14, 2015

I've opened a PR for this: #784

One concern with a change like this is that it won't play nicely with custom GHC builds, so we need to consider how exactly to deal with that situation.

How about handing this case with the --skip-ghc-check flag?

mgsloan added a commit that referenced this issue Aug 14, 2015
* Requires the exact GHC version specified by the snapshot.

* Deprecates major-version (ghc-7.10, ghc-7.8) resolvers, but still
  supports them.  They resolve to the newest installed / available ghc
  matching that number.

* Changes the format of stack-setup.yaml, and so changes which URL is
  used to find it (in order to not break old stack versions).

* Refactors ensureTool code, as it already had a lot of special cases,
  which I found confusing.  Main cause is that I needed to pass in a
  'CompilerVersion' instead of 'Version', but just for installing ghc,
  not for git.

* Introduces a 'CompilerVersion' type, and changes some naming to
  specify that compiler versions are being passed around rather than ghc
  versions.  The change could be a simpler without this, but this will
  be helpful for GHCJS support.
@borsboom
Copy link
Contributor

@borsboom borsboom commented Aug 14, 2015

One very common case of a custom GHC build is when using Docker images, which almost always contain a custom GHC build.

@mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Aug 14, 2015

It's fine if it's a custom build as long as ghc --version claims to be an ordinary version. I just tried it out with fpco/stack-ghcjs-build:lts-2.14, and stack exec -- ghc version yields 7.8.4. I think @snoyberg is referring to things like ghc development builds.

@snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Aug 14, 2015

How about this heuristic: match as many components of the version as are
specified. If the user requests GHC 7.10.2, we could also match 7.10.2.12345

On Fri, Aug 14, 2015, 6:35 AM Emanuel Borsboom notifications@github.com
wrote:

One very common case of a custom GHC build is when using Docker images,
which almost always contain a custom GHC build.


Reply to this email directly or view it on GitHub
#736 (comment)
.

@mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Aug 14, 2015

Hmm, I like it! I'll update the PR with that. This means we won't actually be deprecating the major-version resolvers, right? Heck, ghc-7 will work too. If we really wanted to go all the way, just plain ghc could be a resolver.

@snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Aug 14, 2015

I have one other idea to consider: what if we had:

allow-minor-compiler-bump: BOOL

If true, then when you specify ghc-7.10.2, it will have the behavior of allowing 7.10.3 as well. We could even in theory generalize this to:

exact-compiler-components: INT

Where a value of 2 would mean the same as allow-minor-compiler-bump: true, but a value of 1 would mean GHC >= 7.10.2 && < 8.

I'm not convinced either of these is a good idea tbh.

@mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Aug 14, 2015

One mildly problematic aspect of this is that GHCJS will have two version numbers (GHCJS version, GHC version it's built against), and this policy would be applied to both.

I'm thinking that the heuristic you mentioned earlier is enough to handle the common case for custom builds, and --skip-ghc-check flag can handle the rest. Sound good?

The only thing I don't like about the "matching prefix" approach is that it doesn't allow you to specify a fixed version, as there's always an implicit wildcard. So, one possibility would be to have exact-compiler-version, which defaults to false. I'm not sure if anyone would actually need this, though. Maybe wait for demand?

@snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Aug 14, 2015

Sorry to beat this to death... but I don't like the idea of relying on skip-ghc-check to address use cases we expect users to have regularly. Here's what I see:

For stack setup, implicitly adding .* to the end of the version is the right thing to do. So stack setup 7.8 means "latest minor release in 7.8,stack setup 7.8.4` would means exactly 7.8.4 (since our stack-setup.yaml won't have 7.8.4.X in it), etc. I don't think any configuration for this is necessary, I can't see a use case where this behavior wouldn't be exactly what the user wants.

For the GHC check, the easy case is where the user explicitly states the GHC version desired (i.e., the ghc- resolver or the custom resolver). However, when using a Stackage snapshot (lts- or nightly-), the GHC version will always have three components in it. Here are the use cases I can see:

  • Want to force exactly the same version
  • Want to force the same minor version, but a patch (as done in a Docker image) should work
  • Want to use the same major version, and the current or later minor version
  • Want to test a completely different GHC version (e.g., HEAD) with the current snapshot. This is the case skip-ghc-check is designed for, and should continue to serve

I think we should have a sum type, compiler-check, that will include the three cases above, and in theory more in the future. Then we can say:

resolver: lts-3.0
compiler-check: match-minor # the default
# compiler-check: match-exact
# compiler-check: newer-minor

As for the GHCJS case, I see two options: either we say that using the same compiler-check for both GHC and GHCJS is valid (seems valid to me offhand), or we allow some kind of mapping for it, e.g.:

resolver: ghcjs-..... # I forgot the exact syntax
compiler-check:
    ghc: match-minor
    ghcjs: newer-minor
@mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Aug 14, 2015

That seems like a good solution to me, lets go with this approach.

mgsloan added a commit that referenced this issue Aug 14, 2015
* Stack now defaults to matching the minor version specified in the
  resolver / snapshot, whereas before only the major version was
  significant.

* Adds a 'compiler-check' field which specifies a policy for checking
  the GHC version.

* Changes the format of stack-setup.yaml, and so changes which URL is
  used to find it (in order to not break old stack versions).

* Refactors ensureTool code, as it already had a lot of special cases,
  which I found confusing.  Main cause is that I needed to pass in a
  'CompilerVersion' instead of 'Version', but just for installing ghc,
  not for git.

* Introduces a 'CompilerVersion' type, and changes some naming to
  specify that compiler versions are being passed around rather than ghc
  versions.  The change could be a simpler without this, but this will
  be helpful for GHCJS support.
mgsloan added a commit that referenced this issue Aug 14, 2015
* Stack now defaults to matching the minor version specified in the
  resolver / snapshot, whereas before only the major version was
  significant.

* Adds a 'compiler-check' field which specifies a policy for checking
  the GHC version.

* Changes the format of stack-setup.yaml, and so changes which URL is
  used to find it (in order to not break old stack versions).

* Refactors ensureTool code, as it already had a lot of special cases,
  which I found confusing.  Main cause is that I needed to pass in a
  'CompilerVersion' instead of 'Version', but just for installing ghc,
  not for git.

* Introduces a 'CompilerVersion' type, and changes some naming to
  specify that compiler versions are being passed around rather than ghc
  versions.  The change could be a simpler without this, but this will
  be helpful for GHCJS support.
@mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Aug 14, 2015

@snoyberg I've implemented this and tested it some, seems to work! I haven't tried it against any funky installed ghc versions, but the compiler-check stuff seems to work correctly.

@snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Aug 14, 2015

Looking good. Let's move discussion (if necessary) to the PR (#784). Closing this.

@snoyberg snoyberg closed this Aug 14, 2015
@snoyberg snoyberg removed the in progress label Aug 14, 2015
snoyberg added a commit that referenced this issue Aug 14, 2015
@hnsl
Copy link
Author

@hnsl hnsl commented Aug 14, 2015

Great work guys. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.