[rfc] only unpack package versions within preferred-versions #1391

[luigy]

first pass at #1391

• fetches version present in snapshot otherwise falls back to hackage
• caches preferred-versions from index
• only versions within preferred-versions are fetched unless explicitly asked by package identifier
• added --latest flag to fetch latest version from hackage regardless of resolver

TODO:

• Don't copy loadBuildPlan from Stack.BuildPlan into Stack.Fetch
• Update Stack.Upgrade to only consider non-deprecated versions.

New behavior:

λ cat stack.yaml
resolver: lts-5.4

λ stack unpack stack
Unpacked stack-1.0.2 to /Users/luigy/stack-1.0.2/

λ stack --resolver lts-3.0 unpack stack
Unpacked stack-0.1.3.0 to /Users/luigy/stack-0.1.3.0/

λ stack unpack --latest stack
Unpacked stack-1.0.4.1 to /Users/luigy/stack-1.0.4.1/

λ stack unpack stack-9.9.9
Unpacked stack-9.9.9 to /Users/luigy/stack-9.9.9/

[mgsloan]

Why not filterBy without the prime?

[mgsloan]

LGTM so far! Are those checkboxes supposed to be checked? (in the issue description)

It'd be cool, though not required, to have tests that the snapshot version is unpacked, that --latest works, and the preferred versions stuff works

[mgsloan]

Also, note that it's failing to build for 7.8. Without looking at the error, most of the time I get that it's due to needing to use liftM instead of fmap in some circumstances.

[luigy]

Are those checkboxes supposed to be checked? (in the issue description)

Maybe :)
For the first one I was opening it up for suggestions about the duplication of loadBuildPlan
I have taken care of the second one, but not sure about the name getLatestApplicablePackageCache

I have also added tests and have a, probably too fragile, way to get the latest non-deprecated from hackage to check against stack unpack --latest stack test result.
Maybe just make sure that it exits successfully ?

[mgsloan]

Yeah, I'd be fine with just making sure that it exits successfully for the latest case. It's sufficient to just check that stack unpack --latest stack does not download stack-9.9.9

[mgsloan]

Thanks for adding a test!

For avoiding copying loadBuildPlan, how about moving the code for getting the versions from snapshot packages into Main.hs? I don't mind having some logic in Main.hs, particularly if it's concerned with a CLI convenience.

I have an enhancement suggestion: how about only loading the configuration / build plan if there are package names to resolve. So if the user does stack unpack specific-version-1.0, we don't even need a valid stack.yaml. This is another reason to put this logic in Main.hs, since that's where the configuration gets loaded.

Ooh, and I just thought of something. I think we want stack unpack to also pay attention to extra-deps. This would mean using something like loadSourceMap AllowNoTargets defaultBuildOpts.

Also, I don't think this is strictly necessary, but it would also be good to have clear messaging in the following case:

• If the user attempts to unpack a local package (like stack unpack pkg, and pkg is one of the project's packages):
  • If the package exists on hackage, and --latest is specified, warn that we're unpacking a version from hackage, and not using the local one.
  • If the package exists on hackage, but --latest is not specified, error out explaining that it's a local package and to download from hackage, a specific version or --latest is required.
  • If the package does not exist on hackage, error out explaining that the user tried to unpack a local package.

[luigy]

I'm back! let's 🚢 this one! 🎉 🕐

For avoiding copying loadBuildPlan, how about moving the code for getting the versions from snapshot packages into Main.hs?

I like! done ✔️

I have an enhancement suggestion: how about only loading the configuration / build plan if there are package names to resolve.

Ommiting the buildplan! done ✔️

Hmm, omitting the configuration ✖️
What about private package index? upcoming extensible snapshots?
It makes sense but, I have to think a bit more about this one. I'm in between convenience vs. being explicit about it. We should look more into the global --no-config flag for that 😄

Ooh, and I just thought of something. I think we want stack unpack to also pay attention to extra-deps.

Meaning that it should fetch the extra-dep's version if it shadowed the snapshot one when only given a package name?

Makes sense! This behavior goes more inline with what you were suggesting in #1441, right?

‼️ In any case, we should start trying to make sure that we are maintaining a consistent behavior across commands when taking into account the config 🔍
For example, should #1441 also accept stack ghci text-1.2.0.0 similar to stack unpack?
Similar to the expected behavior of #1261

If the package exists on hackage, and --latest is specified, warn that we're unpacking a version from hackage, and not using the local one.

👍

If the package exists on hackage, but --latest is not specified, error out explaining that it's a local package and to download from hackage, a specific version or --latest is required.

👍 makes sense in order to show a consistent behavior when handling the config. Currently, I was ignoring extra-deps and just going straight to the snapshot. Also, once again, if they really want the snapshot version a --no-config flag can come in handy

If the package does not exist on hackage, error out explaining that the user tried to unpack a local package.

👍 but also considering the config's custom indices/snapshots, right? same goes for the first suggestion

[mgsloan]

What about private package index? upcoming extensible snapshots?

Good point! The private package index option does mean that the config should always be loaded.

Ooh, and I just thought of something. I think we want stack unpack to also pay attention to extra-deps.
Meaning that it should fetch the extra-dep's version if it shadowed the snapshot one when only given a package name?

Yup

For example, should #1441 also accept stack ghci text-1.2.0.0 similar to stack unpack?
Similar to the expected behavior of #1261

Yes indeed!

If the package does not exist on hackage, error out explaining that the user tried to unpack a local package.
👍 but also considering the config's custom indices/snapshots, right? same goes for the first suggestion

Yep!

[mgsloan]

@luigy Hey! I'm doing a bit of a PR cleanup sweep, how's it going?

I think potentially the discussion above is making this a bit complicated. How about lets get an initial version of this in - just the bit with the code cleanup

[mgsloan]

Since this has languished for a while, I've added a link to it from the issue.

Thanks for working on this @luigy, when we return to that issue, this code will be a helpful reference!