commercialhaskell / stack

The Haskell Tool Stack
http://haskellstack.org
BSD 3-Clause "New" or "Revised" License
4k stars 843 forks source link

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

Closed ndmitchell closed 6 years ago

ndmitchell commented 6 years ago

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 commented 6 years ago

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.

ndmitchell commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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:

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

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 commented 6 years ago

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 commented 6 years ago

PR #3601 is now open

snoyberg commented 6 years ago

This was already addressed by #3601, closing.