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

Default to using a stack-installed, isolated GHC #2537

Merged
merged 6 commits into from
Oct 16, 2016

Conversation

sjakobi
Copy link
Member

@sjakobi sjakobi commented Aug 28, 2016

Closes #2221.

@sjakobi
Copy link
Member Author

sjakobi commented Aug 28, 2016

I expect that this change will break a lot of CI scripts, e.g. those that use hvr's repo to install various GHC versions. Should I highlight this more in the Changelog?

@@ -861,7 +861,7 @@ ghcVariantParser hide =
readGHCVariant
(long "ghc-variant" <> metavar "VARIANT" <>
help
"Specialized GHC variant, e.g. integersimple (implies --no-system-ghc)" <>
"Specialized GHC variant, e.g. integersimple" <>
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's still implied, no? Or is it now legal to use --system-ghc=... --ghc-variant?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's still implied, no?

I guess that depends on how one understands "implies": I read it as "changes default settings" (in this case from --system-ghc to --no-system-ghc), while you seem to read it like "requires".

I haven't been able to test --ghc-variant <sth-non-standard> on Ubuntu-16.04 (I didn't try very hard) but according to #816 it's intended to be used only with --no-system-ghc.

I don't think --ghc-variant should silently override a user's --system-ghc setting, so how about I add (incompatible with --no-system-ghc) to the CLI help line?
I'd also add a check that errors out if both --ghc-variant X and --system-ghc are passed in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both your proposals are good. FWIW I understand "implies" like you do, I had just misunderstood the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I meant to write (incompatible with --system-ghc) above BTW.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I totally didn't notice and the discussion made sense anyway. But I need coffee...

@borsboom
Copy link
Contributor

I expect that this change will break a lot of CI scripts, e.g. those that use hvr's repo to install various GHC versions. Should I highlight this more in the Changelog?

I think this probably should at least go in the Major Changes section to make it more likely to be noticed by someone skimming the changes.

@borsboom
Copy link
Contributor

I don't think --ghc-variant should silently override a user's --system-ghc setting, so how about I add (incompatible with --no-system-ghc) to the CLI help line?
I'd also add a check that errors out if both --ghc-variant X and --system-ghc are passed in.

I agree.

@sjakobi
Copy link
Member Author

sjakobi commented Aug 29, 2016

@YPares: I'm not sure how this change affects Nix users. Could you take a look at that?

@sjakobi
Copy link
Member Author

sjakobi commented Aug 29, 2016

I expect that this change will break a lot of CI scripts, e.g. those that use hvr's repo to install various GHC versions.

LOL, here we go: https://travis-ci.org/commercialhaskell/stack/jobs/155970348

@borsboom
Copy link
Contributor

The sanity integration test also appears to have broken. See https://jenkins-public.fpcomplete.com/job/stack-integration-tests/225/consoleFull

@Blaisorblade
Copy link
Collaborator

Auto-installing GHC when needed would silently fix the issue with setup scripts. I assume that's a bad idea for some reason, but maybe that should be reconsidered? As long as Stack can be trusted to install a good GHC, that seems a fine default. The main downside, IIUC, is if GHC can't be trusted to install a good GHC, says on Linux platforms (HVR mentioned issues with Ubuntu 16.10 recently — no GHC bindist works there).

@YPares
Copy link
Collaborator

YPares commented Aug 30, 2016

@sjakobi Well, for it to work with nix I have to pass --system-ghc.
I think system ghc should be automatically activated if nix is.

@borsboom
Copy link
Contributor

Auto-installing GHC when needed would silently fix the issue with setup scripts. I assume that's a bad idea for some reason, but maybe that should be reconsidered?

I remember we hashed this out a long time ago, and it boiled down to most people having a preference for not being "surprised" by Stack going ahead and installing GHC without being asked to. There's no technical reason one way or the other. The --install-ghc flag switches to the other behaviour.

@snoyberg
Copy link
Contributor

Just some ideas to throw out there. There are a number of people who really prefer to use system-installed GHCs. While it may be problematic as a default, we shouldn't make these people suffer through typing --system-ghc constantly, or have to edit all of their local stack.yaml files. Instead, what if we:

  • Recognize a global configuration override to what the default for system-ghc is (true or false)
  • Add a new command for changing that default (strawman: stack set system-ghc or stack set no-system-ghc, definitely up for debate)
  • In the code which gives the user the message about running stack setup, also check if ghc is on the PATH and, if so, give a message about running stack set system-ghc

I think this will give us a situation where unexpecting users don't accidentally use a broken system install and determined users can easily use that install. CI scripts will suffer for this (which is really unfortunate), but at least the fix will be as easy as adding stack set system-ghc.

@borsboom
Copy link
Contributor

@sjakobi Well, for it to work with nix I have to pass --system-ghc.
I think system ghc should be automatically activated if nix is.

The same probably applies to Docker.

@borsboom
Copy link
Contributor

CI scripts will suffer for this (which is really unfortunate), but at least the fix will be as easy as adding stack set system-ghc.

While we're at it, perhaps also including stack set install-ghc (or something like it) to enable Stack automatically installing GHC without needing to run stack setup should be added.

@Blaisorblade
Copy link
Collaborator

stack set X, Y or Z

If we do that, should we hardcode an ad-hoc subset of config.yaml or support arbitrary keys? If we decide to hardcode some, let's pick an interface that is a subset of what we have later. (git config might be an inspiration here).

@borsboom
Copy link
Contributor

pinging @drwebb, who has done some thinking and work around supporting stack set or stack config-like commands in the past

@sjakobi
Copy link
Member Author

sjakobi commented Aug 31, 2016

Instead, what if we:

  • Recognize a global configuration override to what the default for system-ghc is (true or false)

Do you mean https://docs.haskellstack.org/en/stable/yaml_configuration/#system-ghc or are you asking for something different?

  • Add a new command for changing that default (strawman: stack set system-ghc or stack set no-system-ghc, definitely up for debate)

To modify project stack.yamls we already have the stack config set command which currently only supports the resolver option. For CI scripts it might be enough to extend this command to support the system-ghc option so users could modify the system-ghc setting for the current project.

If we want to modify the global configuration (~/.stack/config.yaml) we could maybe extend stack config with a --global flag similarly to git config. Some users may think though that the --global flag is for the "implicit global project`…

  • In the code which gives the user the message about running stack setup, also check if ghc is on the PATH and, if so, give a message about running stack set system-ghc

👍. We should probably check whether it's the right version of GHC too.

While we're at it, perhaps also including stack set install-ghc (or something like it) to enable Stack automatically installing GHC without needing to run stack setup should be added.

I agree. To be clear, the install-ghc config option already exists.


BTW, I believe that all the "non-project config" options can currently also be specified in project-local stack.yamls. If we agree that this is the way it should be, I'd fix the docs which currently claim that these options can only be specified in the global configuration. Oh, and the /etc/stack/config.yaml path is deprecated and should be removed, right?

@snoyberg
Copy link
Contributor

Sounds good, I'd simply forgotten that the global configuration would be included (shows how long since I've worked on the codebase...).

@sjakobi
Copy link
Member Author

sjakobi commented Aug 31, 2016

If we do that, should we hardcode an ad-hoc subset of config.yaml or support arbitrary keys?

I think ultimately we want to support arbitrary keys. #115 somewhat tracks this idea, although currently only for stack.yamls.

If we decide to hardcode some, let's pick an interface that is a subset of what we have later. (git config might be an inspiration here).

👍

@sjakobi
Copy link
Member Author

sjakobi commented Aug 31, 2016

@snoyberg

I'd simply forgotten that the global configuration would be included

I don't understand what you're trying to express here. Can you expand a bit?

@sjakobi
Copy link
Member Author

sjakobi commented Aug 31, 2016

Also, to keep the stack config set syntax "regular", I'd propose to write stack config set system-ghc false. A shorter syntax for boolean options like stack config set no-install-ghc could be added later if deemed worth the hassle.

@snoyberg
Copy link
Contributor

Please don't put too much weight on my comments, the syntax was intended as strawmen, and you're in a better position to decide on the exact syntax.

@snoyberg
Copy link
Contributor

Just expressing that my mind is porous and I forgot many relevant details
of how Stack works. You really shouldn't pay much attention to me, I'm
causing more harm than good here.

On Wed, Aug 31, 2016, 8:55 PM Simon Jakobi notifications@github.com wrote:

@snoyberg https://github.com/snoyberg

I'd simply forgotten that the global configuration would be included

I don't understand what you're trying to express here. Can you expand a
bit?


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#2537 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADBB-UAzWtyZCnyZAvzupLjY8MqNLKSks5qlabGgaJpZM4Ju-Op
.

@Blaisorblade
Copy link
Collaborator

If we want to modify the global configuration (~/.stack/config.yaml) we could maybe extend stack config with a --global flag similarly to git config. Some users may think though that the --global flag is for the "implicit global project`…

You could have --config and --global (or the longer --global-config and --global-project if needed).

@sjakobi
Copy link
Member Author

sjakobi commented Aug 31, 2016

Just expressing that my mind is porous and I forgot many relevant details of how Stack works.

Gotcha! ;)

You really shouldn't pay much attention to me, I'm causing more harm than good here.

Not at all! Your design input here is very valuable even if you aren't aware of each and every new stack feature! The stack set system-ghc idea is very good and should make the system-ghc transition much more tolerable for users! Please keep the ideas coming!

@sjakobi sjakobi force-pushed the 2221-default-to-no-system-ghc branch from 1184c74 to 4f29f24 Compare September 1, 2016 00:10
@sjakobi
Copy link
Member Author

sjakobi commented Sep 29, 2016

No, I don't think so. Installing/removing Nix is really simple and everything Nix is already isolated (in /nix and in your home). After that, you just have to source the nix profile script and then you're good to run stack --nix.

Ok, I'll give that a try a then.

Thanks for testing!

@mgsloan
Copy link
Contributor

mgsloan commented Sep 30, 2016

@sjakobi Sounds like you have a handle on it :) Awesome!

@Blaisorblade
Copy link
Collaborator

Could you, @YPares, maybe write a short guide on how to use that (for Nix & Docker noobs) and add it to the project documentation section?

Installing/removing Nix is simpler and everything Nix is already isolated (in /nix and in your home).

While it's probably not your responsibility, if (big if) we're all supposed to be able to test Nix support, could you please still document a recommended installation method? As a Nix noob, googling for a few minutes doesn't tell me what I need, and I'm afraid I can't risk messing up my system.

Among the top 3 results for "installing nix", I assume you mean neither of https://nixos.org/wiki/Installing_Nix_on_Debian or https://nixos.org/wiki/How_to_install_nix_in_home_(on_another_distribution), and suspect that I should follow http://nixos.org/nix/download.html. That still mentions no "nix profile script" and neither does http://nixos.org/nix/manual/#chap-quick-start. And neither reassures me Nix will leave everything else alone—in fact, the example below makes no sense since none of the commands shown should be able to modify the PATH:

$ which hello
/home/eelco/.nix-profile/bin/hello

@YPares
Copy link
Collaborator

YPares commented Oct 4, 2016

@Blaisorblade The nix documentation https://github.com/commercialhaskell/stack/blob/master/doc/nix_integration.md indeed refers to http://nixos.org/nix/download.html .
I prefere not to duplicate any installation method in stack documentation (as it's nix maitainers responsibility to update http://nixos.org/nix/download.html if this installation method changes).
This page doesn't mention the nix profile script (the one in ~/.nix-profile/etc/profile.d/nix.sh) because the installation process already adds its sourcing to your bashrc.

But I'll add this part to the doc anyway, yes.

YPares added a commit that referenced this pull request Oct 4, 2016
@Blaisorblade
Copy link
Collaborator

This page doesn't mention the nix profile script (the one in ~/.nix-profile/etc/profile.d/nix.sh) because the installation process already adds its sourcing to your bashrc.

Thanks for pointing me to stack docs on nix integration, they seem really great, and installing Nix is not remotely invasive, unlike I feared.

@sjakobi
Copy link
Member Author

sjakobi commented Oct 9, 2016

What's still missing here is mostly a fix for the integration tests. I'm not sure what to do about them yet, so if anyone has an idea, please chime in!

I'll probably get back to this tomorrow or on Tuesday.

@Blaisorblade
Copy link
Collaborator

Sorry for the basic question, but for the integration tests, why isn't stack config set --global system-ghc true enough?

@mgsloan
Copy link
Contributor

mgsloan commented Oct 10, 2016

Looks pretty good to me so far @sjakobi ! I think this can be merged soon.

I do not see the code changes for config set. If it's not ready soon, feel free to make a P0 issue for implementing the config modification changes portion of this.

@sjakobi
Copy link
Member Author

sjakobi commented Oct 10, 2016

Sorry for the basic question, but for the integration tests, why isn't stack config set --global system-ghc true enough?

I guess you're right, for fixing the integration tests that might just about be enough!

We could still add tests that use a sandboxed GHC later.

I do not see the code changes for config set.

I did these in separate PRs / commits: #2675 and 0bdab6e.

@sjakobi
Copy link
Member Author

sjakobi commented Oct 10, 2016

Also RE stack config set:

In my implementation, the --global flag is only available for some fields, e.g. not for resolver.

Sadly this means that stack config set --global system-ghc true is a syntax error:

~/t/hkl $ stack config set --global system-ghc true
Invalid option `--global'

The --global flag must come after the key: e.g. stack config set system-ghc --global true or stack config set system-ghc true --global.

@Blaisorblade
Copy link
Collaborator

Blaisorblade commented Oct 10, 2016

Sadly this means that stack config set --global system-ghc true is a syntax error:

That's an implementation restriction right? Can you document it as such for now?
If there's some design reason why --global makes no sense for resolver maybe we should reconsider the (EDIT) command-line API.

Address option compatibility issues around the new default setting:

* Add notes to ghc-variant and setup --reinstall help texts
* Add a check to catch uses of ghc-variant in combination with
  no-system-ghc
Error out if `--no-system-ghc` is specified in a Nix-enabled configuration.
@sjakobi sjakobi force-pushed the 2221-default-to-no-system-ghc branch from c9a0917 to 76a6562 Compare October 13, 2016 10:15
@sjakobi
Copy link
Member Author

sjakobi commented Oct 13, 2016

@Blaisorblade

Sadly this means that stack config set --global system-ghc true is a syntax error:

That's an implementation restriction right? Can you document it as such for now?
If there's some design reason why --global makes no sense for resolver maybe we should reconsider the (EDIT) command-line API.

I have opened #2707 to discuss this. Is this what you meant by "Can you document it as such for now?" or would you like to see other documentation of this too?

@sjakobi
Copy link
Member Author

sjakobi commented Oct 13, 2016

I think this issue is ready for a final review now. Sorry it took me so long!

@Blaisorblade
Copy link
Collaborator

Is this what you meant by "Can you document it as such for now?" or would you like to see other documentation of this too?

Read #2707, so I get it: using --global is not supported and can't be supported, because it doesn't make sense. It's not an implementation restriction, that is, something sensible we just don't handle (yet).

We need user docs for stack config—the more pitfalls, the more we need those docs.

@mgsloan
Copy link
Contributor

mgsloan commented Oct 16, 2016

@sjakobi These changes look great! I just realized that the stack config set system-ghc stuff is already on master. So this is definitely good to go

@mgsloan mgsloan merged commit ff9bd61 into master Oct 16, 2016
@sjakobi sjakobi deleted the 2221-default-to-no-system-ghc branch October 16, 2016 15:40
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.

6 participants