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

Local extra-dep should not be considered $locals #3574

Closed
ndmitchell opened this Issue Nov 13, 2017 · 7 comments

Comments

Projects
3 participants
@ndmitchell
Contributor

ndmitchell commented Nov 13, 2017

Using stack installed with --git on Windows 10 64bit, version:

Version 1.6.0, Git revision 46121be1b96465f1164e3f84cafa19c7369da9cc x86_64 hpack-0.18.1

Given stack-bug.zip, that defines in the stack.yaml:

packages:
- location: foo
  extra-deps: true

ghc-options:
  $locals: -bob

When compiling foo I don't expect the flag -bob to be applied as foo is an extra-dep, and thus not a local package. However, when I stack build I get an error about:

ghc.EXE: unrecognised flag: -bob
@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Nov 14, 2017

extra-deps is perhaps not the best name, a bit of a historical artifact. It would be nice to improve the stack documentation to describe the terminology properly. Or, perhaps, change the behavior to be less surprising.

"extra-dep" essentially just means "not a target by default". Currently, they are always considered local packages. Once implicit snapshots is implemented, this wouldn't be the case and the local package vs snapshot package will make even less sense. So I anticipate that the local package distinction will get dropped.

I think $targets is closer to the behavior of what you're looking for, but it will also be applied in the case that you do stack build foo, but won't be if you just do stack build.

Not sure what to call the non-extra-deps. default-targets? I guess could add another set of packages specified by the config, based on not being an extra-dep.

@mgsloan mgsloan added this to the Support milestone Nov 14, 2017

@ndmitchell

This comment has been minimized.

Contributor

ndmitchell commented Nov 14, 2017

Hmm, extra-deps is indeed a terrible name - we've used it completely incorrectly. Can I suggest adding definitions of $locals and the other $variables to yaml_configuration.md. Note that defining custom ghc-options makes something a $locals (I think). I did read the docs, and came to the conclusion (perhaps from extra-deps elsewhere on the page) that the behaviour is wrong.

FWIW $default-targets was exactly what I wanted, but following #3573 it's not as important, and given the extra-deps terminology elsewhere $default-targets would be reasonably confusing.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Nov 22, 2017

First things first: the correct syntax is to say extra-dep: true instead of extra-deps: true. In fact, if I run stack build in this directory, I see:

Warning: /Users/michael/Desktop/stack.yaml: Unrecognized field in PackageEntry: extra-deps

Changing to extra-dep: true gets me:

Error parsing targets: The project contains no local packages (packages not marked with 'extra-dep')

And then stack build foo gets the error in question.

I think this is just a bug in implementation: I would not consider any extra-dep packages to be locals, and think it's just a bug in the code base. My understanding and reading of the docs have always gone in that direction. @mgsloan are you inherently opposed to just fixing the implementation such that $locals only applies to actual local packages?

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented Nov 23, 2017

@snoyberg I am not opposed to that. I think part of the root of this is that IIRC, the LocalPackage type is also used for git packages, I suppose since the code is always available locally. It might actually be nice to reduce the distinctions between snapshot and local packages, keeping track of unpacked snapshot source. I don't think this would work for wired in packages / other packages that come with ghc, so that could be tricky.. It'd have the following advantages:

  • Potentially implement #1441 without needing to do an unpack
  • Would make it a bit easier to support "jump to definition" for dependencies in intero commercialhaskell/intero#231

In fact, I think it would be good to take this as an opportunity to consolidate / define stack's terminology around different sorts of packages. Currently we have: the following terms

  • snapshot package: this is usually pretty clear, but can get blurry when it comes to extra-deps
  • extra-dep package: I interpret this as meaning "non default target". It's a bit of a weird name imho, certainly used to it by now.
  • local package: ???
  • target package: specified on the CLI, or implicitly selected.
  • remote package: I think this means packages from repos? You'd think local vs remote would be antonyms, but they don't seem to be. I suppose that's the crux of this issue.
  • hackage package: really means "from the package index / repository"
  • stackage package: means a package from a stackage snapshot. Something currently tracked by stackage
  • wired-in package: built into ghc, present in global DB

Would also be good to include along with the docs for this terminology, also the terminology for DBs, even though that's an implementation detail. It would be good to explain global DB vs snapshot DB vs local DB.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Nov 23, 2017

I think we're mostly on the same page, see #3352. For now, I'm going to make changes necessary so this specific case passes.

snoyberg added a commit that referenced this issue Nov 23, 2017

snoyberg added a commit that referenced this issue Nov 23, 2017

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Nov 23, 2017

PR #3601 is now open

snoyberg added a commit that referenced this issue Nov 23, 2017

mgsloan added a commit that referenced this issue Nov 26, 2017

@snoyberg snoyberg added this to Needs review in Pantry Aug 7, 2018

@snoyberg snoyberg moved this from Needs review to To do in Pantry Aug 7, 2018

@snoyberg snoyberg moved this from To do to Backlog in Pantry Aug 7, 2018

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Aug 23, 2018

This was already addressed by #3601, closing.

@snoyberg snoyberg closed this Aug 23, 2018

Pantry automation moved this from Backlog to Done Aug 23, 2018

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