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

Unify behaviour of flags and ghc-options #849

Closed
borsboom opened this Issue Aug 25, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@borsboom
Contributor

borsboom commented Aug 25, 2015

In particular, ghc-options in stack.yaml should also promote snapshot dependencies to extra-deps (to avoid breaking the snapshot sandbox). This also will open the door to more easily adding other options that effect what is passed to runhaskell Setup.hs configure.

@borsboom borsboom self-assigned this Aug 25, 2015

@borsboom borsboom added this to the 0.2.0.0 milestone Aug 25, 2015

@borsboom borsboom added the blocked label Nov 1, 2015

@borsboom

This comment has been minimized.

Contributor

borsboom commented Nov 1, 2015

This may be subsumed by #1265 (Unify extra-deps and extensible snapshots)

@borsboom borsboom modified the milestones: P2: Should, P1: Must Nov 1, 2015

@borsboom borsboom removed their assignment Nov 30, 2015

mgsloan added a commit that referenced this issue Apr 30, 2016

Add ghc opts and allow-newer for custom snaps
See #1265, some of the code refactor related to #849 and #863

mgsloan added a commit that referenced this issue May 1, 2016

Add ghc opts and allow-newer for custom snaps
See #1265, some of the code refactor related to #849 and #863

mgsloan added a commit that referenced this issue May 4, 2016

Add ghc opts and allow-newer for custom snaps
See #1265, some of the code refactor related to #849 and #863
@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Oct 24, 2016

We have a couple cases where a ghc-option must be applied to all packages. I'm not even sure how feasible that is considering that we have wired-in packages (that come with GHC / GHCJS).

Nonetheless, it makes a lot of sense for ghc-options to actually be declarative rather than having the meaning be sensitive to what's already installed. I think this is one of the ugliest parts of stack right now.

I agree that this is likely to be subsumed by #1265

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Mar 18, 2017

I've spent some time looking into this, and figured I'd note down the current state of things:

The flags specified in the stack.yaml must refer to packages declared in the config. So in this case promotion to an extra-dep is the responsibility of the user. See checkFlagsUsed in Stack.Build.Source. The automatic promotion to extra-dep only happens for CLI flags (convertSnapshotToExtra).

Flags specified in the config and cli replace those specified in the snapshot. Conversely, ghc-options get added to those specified in the snapshot. See extraDeps3 in Stack.Build.Source.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jul 5, 2017

I specifically avoided making this change when doing my refactoring to match the previous behavior, together with FIXMEs in the code to reconsider. I'm happy to see this issue. After the PR is merged (#3249), I'll attack this with a separate PR.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Aug 8, 2017

Alright, here's a bit of a wrinkle from a design standpoint. We've got two kinds of ghc-options overrides:

  • By package
  • For all packages

For by package options (specified in the stack.yaml file), I think it's pretty straightforward that we should promote packages to the local database when they are set. It's less obvious was good behavior is for options applied to all packages (either from the stack.yaml file or from the command line). Here are the choices I see:

  1. If any such options are specified, promote all packages to the local database. This will result in a lot of rebuilding for confusing reasons to the user
  2. Modify behavior to only apply the ghc-options to already-local packages, which is a perhaps significant behavior change
  3. Continue the current behavior: options are applied and silently affect the snapshot database

It doesn't seem like (1) is a good option, even if it's the most straightforward. And we know that (3) is troublesome from a reproducibility standpoint. How about a modified version of (2)?

  • Introduce a new setting: apply-ghc-options, with a few different values
  • all-no-promote uses the current behavior ((3) above)
  • local-only applies to just local packages ((2) above)
  • Default would be: all-no-promote for now, with a big warning printed about the ambiguity and our intention to change behavior in future versions towards local-only as a default

I'm open to other ideas.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Aug 8, 2017

I just realized we already have an apply-ghc-options field in stack.yaml which works almost exactly like this. The problem is that, currently, it does not affect all package options set in the stack.yaml file. On the other hand, the danger of setting these options is well stated in the configuration docs. So perhaps we can leave the handling of all-package options as is, and just modify the per-package case.

snoyberg added a commit that referenced this issue Aug 8, 2017

snoyberg added a commit that referenced this issue Aug 8, 2017

@borsboom

This comment has been minimized.

Contributor

borsboom commented Aug 8, 2017

I definitely like the local-only option better than the current all-package behaviour.

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Aug 8, 2017

There are certainly cases where folks want to set an option for all packages in the build, such as #2708 .

If any such options are specified, promote all packages to the local database. This will result in a lot of rebuilding for confusing reasons to the user

I thought implicit snapshots meant that this promotion was unnecessary? I thought this was one of the reasons for implicit snapshots. With that, options won't silently affect snapshot packages that get reused by projects that don't specify those options. Moreover, if you've built a package with a particular set of options before, it can be reused.

Not sure where (couldn't find it by search), but I recently saw mention of the idea of allowing ghc-options to apply to different categories of packages, like:

ghc-options:
  # Options applied to all packages
  "*": ...
  # Options applied to local packages
  "$local": ...

Could deprecate "*" in favor of $all for consistency, and to help make people aware of change of semantics here. Not sure about the syntax of prefixing $.

@borsboom

This comment has been minimized.

Contributor

borsboom commented Aug 8, 2017

I thought implicit snapshots meant that this promotion was unnecessary?

Looks like implicit snapshots aren't happening (see #1265 (comment)). It might be worth elaborating those reasons. @snoyberg having been deep in this code recently almost certainly saw problems I hadn't thought of. But yeah, this was the sort of thing I'd hoped implicit snapshots would take care of in an elegant way.

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Aug 8, 2017

Oh darn, that change was rather large so I figured implicit snapshots was happening. From the perspective of the current code, it would indeed be a sweeping change. My responses to the points brought up in the reason not to do it:

The potential for a rather large disk space usage for each time dependencies change

This just means we need to have some variety of GC that deletes packages and DBs based on last use time. This would already be good to have, it's easy for STACK_ROOT to eat up HD over time with stuff that's unlikely to be reused. This would indeed exacerbate the problem, and make automation for it necessary.

Lack of support for extra-deps depending on project packages in that setup

Why not promote extra-deps to local packages in this case? Seems perfectly consistent to me.

snoyberg added a commit that referenced this issue Aug 9, 2017

Merge pull request #3327 from commercialhaskell/849-unify-flags-ghc-o…
…ptions

Promote packages to local database by ghc-options #849
@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Aug 9, 2017

Closing, but see #3329 and #3330.

@snoyberg snoyberg closed this Aug 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment