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

Extensible snapshots #863

Closed
snoyberg opened this Issue Aug 27, 2015 · 16 comments

Comments

Projects
None yet
6 participants
@snoyberg
Contributor

snoyberg commented Aug 27, 2015

I'd like to identify a common syntax for all snapshots to use for markup (LTS, Nightly, and the current custom snapshots), and allow snapshots to extend each other. For example: suppose we're using LTS 3.2, but want to modify the version of aeson to 0.14.0, drop pandoc from the included packages, and switch the compiler from GHC 7.10.2 to GHC 7.10.1. Perhaps this could be accomplished with something like:

# Saved in: my-lts-3.2-mod
extends: lts-3.2
compiler: ghc-7.10.1
packages:
- aeson-0.14.0
drop-packages:
- pandoc
flags:
  aeson:
    # Overrides any previous flag definitions for aeson
    some-flag: true

Longer term: I'm thinking about trying to streamline all of the snapshot code: have a single syntax for LTS, Nightly, and "custom" snapshots, get rid of the specialness of LTS/Nightly (just make it the default available snapshots, like we do with package indices), and have some kind of fallback approach like package indices have right now.

@snoyberg snoyberg added this to the 0.3.0.0 milestone Aug 27, 2015

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Aug 27, 2015

Side point: if we like the syntax we come up with here, perhaps we can use it for making extensible stack.yaml project configurations add well

@3noch

This comment has been minimized.

Member

3noch commented Aug 27, 2015

👍

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Sep 17, 2015

Now that we have sharable binary artifacts between snapshots, I think we can relax some of the original design points behind custom snapshots. Specifically: we previously needed custom snapshots to be semi-mutable so that small changes in a package set wouldn't result in complete recompilation.

Now that that's no longer the case, I'd like to go for a simpler model: assume that a name is a unique mapping to a snapshot definition that never changes. Specifically:

  • If the snapshot definition is stored remotely (http:// or https://), then it will only ever be downloaded once
  • If the snapshot definition is in a local file, then we'll use a hash of the file contents to invalidate the cache
  • All library and package database paths will use just the snapshot name
  • There will be some basic sanity checks by nature of how stack works to ensure, e.g., that the right version of a package is used. However, more subtle changes to a package, like a change in its Cabal flags, may not result in a recompilation

I think this is the right set of constraints to make snapshots efficient to implement, convenient to use, and flexible enough. I'd like some feedback (including questions on what I mean here) before I implement this. Pinging the users of custom snapshots I'm aware of: @gregwebs @feuerbach @hesselink @3noch

@feuerbach

This comment has been minimized.

Contributor

feuerbach commented Sep 17, 2015

Sounds good. The only concern I have is, as the snapshot changes, we'll quietly leak disk space. (Although this is true even for normal snapshots.) Perhaps we need some kind of stack gc.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Sep 17, 2015

A cleanup command is being tracked in #133

@hesselink

This comment has been minimized.

Contributor

hesselink commented Sep 17, 2015

I think this sounds very good. We decided to have our custom snapshots mostly immutable anyway, and the small amount of mutability was only motivated by the long reinstall times of new snapshots. Since those are gone now, making them completely immutable seems like the right choice.

One question: why not treat file urls just like remove urls, copying the file once and then ignoring it?

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Sep 17, 2015

One question: why not treat file urls just like remove urls, copying the file once and then ignoring it?

That would indeed be simpler/more consistent. The reason is to meet the use case of people trying to have semi-mutable snapshots still, which are checked into the repository. Since that's the common use case for mutable snapshots, and it adds very little performance overhead (as opposed to needing to make an HTTP request for every build), it seems like a worthwhile tradeoff.

If no one needs that functionality, however, I'm OK removing it.

@hesselink

This comment has been minimized.

Contributor

hesselink commented Sep 17, 2015

The reason is to meet the use case of people trying to have semi-mutable snapshots still, which are checked into the repository.

This sounds a bit like what we do: we have a snapshot.yaml checked in. When we add packages, we currently just change the file and do nothing else. If we bump versions of dependencies, we change the snapshot file, and also change the snapshot name in stack.yaml. I was under the impression that after this change, the only flow that is supported would be the second one. But now it seems like you'd want to keep support for the first flow as well, is that correct? What would be the benefit? Just avoiding the snapshot name change in the stack.yaml?

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Sep 17, 2015

Benefits would be convenience, and avoiding a very confusing result, namely: if you update the snapshot.yaml and forget to rename it, building on one machine would have a very different result than building on another. By checking the hash, we avoid that corner case.

That case still exists with HTTP-based snapshots, but I think it would be far more unusual to change those, as opposed to changing a local file which is easy to do by mistake.

@hesselink

This comment has been minimized.

Contributor

hesselink commented Sep 17, 2015

That's a good point. Perhaps you could issue a warning in both cases (or the HTTP case only)?

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Sep 17, 2015

What would trigger the warning in the HTTP case? We'd need to perform an
HTTP request in order to detect that something changed, which would be a
major performance hit.

On Thu, Sep 17, 2015, 11:59 AM Erik Hesselink notifications@github.com
wrote:

That's a good point. Perhaps you could issue a warning in both cases (or
the HTTP case only)?


Reply to this email directly or view it on GitHub
#863 (comment)
.

@hesselink

This comment has been minimized.

Contributor

hesselink commented Sep 17, 2015

Yes, I was thinking of doing the HTTP request, but if that's unacceptable performance wise, then it just leaves the choice of allowing mutable snapshots for local files, or not allowing them and having a warning if they change. I'm indifferent, I think we'll treat them as immutable anyway since it doesn't have any downsides now.

@borsboom

This comment has been minimized.

Contributor

borsboom commented Nov 1, 2015

Related: #1265 (Unify extra-deps and extensible snapshots)

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

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented May 1, 2016

This is implemented on https://github.com/commercialhaskell/stack/tree/863-extensible-snapshots :D

Still work in progress, but should work. Those who're keen to use it / find bugs, feel free to give it a try. (though it will put large shas on your custom snapshot storage dirs, not sure if that convention will stick)

Instead of extends, we just re-use resolver. resolver has all the same syntax as normal resolver, so you can indeed chain multiple custom snapshots. Otherwise, the changes to snapshot config are as described in the issue head.

Also adds ghc-options to custom snapshots :D

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 1, 2016

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 added a commit that referenced this issue May 4, 2016

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

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

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

@mgsloan

This comment has been minimized.

Collaborator

mgsloan commented May 7, 2016

Merged, so this is now available on master!

@mgsloan mgsloan closed this May 7, 2016

@borsboom borsboom removed the in progress label May 7, 2016

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented May 7, 2016

Woohoo

On Sat, May 7, 2016, 4:57 AM Michael Sloan notifications@github.com wrote:

Merged!


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#863 (comment)

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