ekmett / contravariant

Haskell 98 contravariant functors
http://hackage.haskell.org/package/contravariant
Other
73 stars 24 forks source link

compatibility with base-4.12 #44

Closed andrewthad closed 5 years ago

andrewthad commented 6 years ago

This is about year down the road, but since Data.Functor.Contravariant will be a part of base-4.12, this package will need to conditionally provide that module depending on the version of GHC/base. If it's inconvenient, I can do this.

RyanGlScott commented 6 years ago

https://phabricator.haskell.org/D4399 just landed, so now it's time to patch contravariant accordingly. (I'll do this.)

RyanGlScott commented 6 years ago

Other libraries that will need to be updated:

svenpanne commented 6 years ago

If I see this correctly, I can't update and test StateVar until a sufficiently recent GHC with Contravariant in base appears at https://launchpad.net/~hvr/+archive/ubuntu/ghc. Is there a plan to upload one?

What are the right conditionals in the Haskell code and in the .cabal file to detect the new Contravariant in base? My guess for Haskell code:

#if MIN_VERSION_base(4,12, 0)
...
#endif

And my guess for a .cabal file:

if impl(ghc >= 8.5)
...

It could very well be the case that I've missed a Wiki page or something like that describing the migration path (just like the ones for the AMP and the FTP).

RyanGlScott commented 6 years ago

If I see this correctly, I can't update and test StateVar until a sufficiently recent GHC with Contravariant in base appears at https://launchpad.net/~hvr/+archive/ubuntu/ghc.

Assuming you're using https://launchpad.net/~hvr/+archive/ubuntu/ghc to test on Travis, then that's correct. I built https://github.com/ekmett/contravariant/pull/45 against a version of GHC HEAD that I built myself, so you could conceivably test against this change today if you were so inclined (but it might just to be wait until that PPA updates).

Is there a plan to upload one?

The GHC HEAD builds from that PPA get updated on a somewhat regular basis, so try checking again after a week or two to see if it's been rebuilt.

What are the right conditionals in the Haskell code and in the .cabal file to detect the new Contravariant in base? My guess for Haskell code:

#if MIN_VERSION_base(4,12, 0)
...
#endif

That sounds right. Alas, the version of base in GHC HEAD is still pegged at 4.11, so we can't use this bit of CPP today. For the time being, I'm using #if __GLASGOW_HASKELL__ >= 805, which is the next-best thing.

And my guess for a .cabal file:

if impl(ghc >= 8.5)
...

At least for StateVar, I don't think you'd need this—depending on base and conditionally defining Contravariant instances in Haskell code should suffice. If you want to depend on the contravariant library for older GHCs (like many people do for tagged/semigroups/bifunctors), then yes, you'd need something like:

if !impl(ghc >= 8.5)
  build-depends: contravariant

It could very well be the case that I've missed a Wiki page or something like that describing the migration path (just like the ones for the AMP and the FTP).

You haven't missed anything—there doesn't exist such a wiki page yet! :)

That being said, this change isn't quite as severe as, say, the AMP or SMP, since we're not going to be making Contravariant a superclass of an existing class in base, so (in theory) not much should break from it.

angerman commented 6 years ago

These are the patches that "work" for me with HEAD (in addition to #45)

https://gist.github.com/angerman/85539d26f2e6eb10815ab170864dfb45

It's obviously wrong, as we re missing a base bump, for these to be proper.

angerman commented 6 years ago

I'd be really happy if we could get a base bump; and a transformer commit, so that I don't need to patch my submodules, but can rather just use a different commit to point at.

RyanGlScott commented 6 years ago

@hvr, would it suffice for the time being to just have a minor version bump for base and patch these libraries locally? We could then do the "right" thing when we have a major version bump in base.

angerman commented 6 years ago

tbqh, I think base should have gotten a major bump when contravariant was partially subsumed; and now that 8.4 is out, I fail to see why we can't bump, as 8.6 is going to be released with a bumped base anyway. But then... maybe I'm missing something.

RyanGlScott commented 6 years ago

If you take the stance that base's version number should be dictated by the PVP, then nothing has happened yet which deserves a major version bump. The biggest change is that we added a new module Data.Functor.Contravariant to base, but that in and of itself does not warrant a major version bump according to the PVP.

Granted, we're in a tight spot since base's version number is often used as a proxy for one's GHC version. So perhaps we should just bite the bullet and perform a major version bump anyway.

phadej commented 6 years ago

FWIW, don't forget about https://github.com/hvr/head.hackage

Now as GHC-8.4.1 is out, it IMHO could concentrate solely on GHC-HEAD

phadej commented 6 years ago

@RyanGlScott also MIN_VERSION_base as well as version bounds can differ on minor (i.e. third) version digit.

RyanGlScott commented 6 years ago

@phadej, that's exactly what we're discussing. We have patches for head.hackage available, but the problem is that they aren't robust, since the CPP they're using applies to more than just GHC HEAD (since we're using MIN_VERSION_base(4,11,0) to guard things).

angerman commented 6 years ago

@phadej right, I wanted to open a PR against head.hackage; but the lack of proper version guards aroudn base, would mean that anyone who uses the head.hackage overlay with 8.4 (or a slightly outdated HEAD checkout) would get all these patches applied :(

EDIT: of course they would be applied, but they would not be guarded correctly. And we can't write correct guards yet.

phadej commented 6 years ago

@RyanGlScott @angerman if we know that there will be breaking changes for GHC-8.6 than I guess one can make major bump already today, it will save some cycles later.

RyanGlScott commented 6 years ago

Alright. In that case, the question goes to @hvr. Should one of us open a patch which just bumps the base version number to 4.12?

In earlier private correspondence, we had discussed waiting until a patch surrounding MonadFailDesugaring had landed to do this. But since folks are asking for this now, perhaps it'd be best just to get that out of the way now.

hvr commented 6 years ago

@RyanGlScott Yeah, let's get a patch up to see what needs bumping; and I'll bring the topic up w/ GHC HQ on tomorrow's call

svenpanne commented 6 years ago

Might it make sense to consider the following base numbering strategy:

In other words: base always has the correct number according to the PvP, even if it was released immediately without further bumps. I think this simple scheme should get us a long way, head.hackage is not always an option, depending on your build/CI system.

In our concrete example, the commit moving Contravariant into base should have contained a minor bump, unless there was already a patch level bump or another minor bump since the 8.4. branch point.

hvr commented 6 years ago

@svenpanne fyi, some time ago I wrote https://ghc.haskell.org/trac/ghc/wiki/Commentary/Libraries/EagerVersionBump

what you're suggesting is already what we're trying to abide to (except sometimes it gets forgotten due to human error... and/or not all libraries' maintainers GHC depends upon follow that suggestion); except it gets a bit more tricky for base as we need to make sure to leave enough space to avoid clashing with the previous GHC releases' base versioning. But in this case it's known that it's very likely that GHC 8.6 will ship w/ base-4.12, so we can just bump to that one now.

svenpanne commented 6 years ago

Just FYI: I've just pushed haskell-opengl/StateVar@ee33c10ab94127fad00c4dd1cee4096fe670b745, but I'm waiting for a base version bump before I release a new version on Hackage (using MIN_VERSION_base instead of __GLASGOW_HASKELL__).

phadej commented 6 years ago

@svenpanne If the only reason is to support GHC-HEAD (which is a moving target!), then as a hackage trustee, I advice you to use https://github.com/hvr/head.hackage, than make speculative releases to Hackage proper.

svenpanne commented 6 years ago

I totally rely on Herbert's make_travis_yml_2.hs script and Travis-CI, so I guess this is already using head.hackage. Nevertheless, base.cabal still claims that base is at 4.11.0.0 (the same version as base without the Contravariant-move), so using MIN_VERSION_base won't help. Unless I'm completely misunderstanding how things are supposed to work, I think base should have been bumped to at least 4.11.1.0 when the move was done, so people have at least a chance to tentatively release new package versions.

hvr commented 6 years ago

so people have at least a chance to tentatively release new package versions.

that's the thing; that's not something that's intended/supported yet; in order to support "tentatively release new package versions" we first need to complete the Hackage release candidate feature (GSOC idea), which was intended to support a staging area for "tentative releases" which could be easily pulled again. The primary Hackage index, its cost model, and its guarantees is not designed/suitable for this and would cause more workload for Trustees and associated infrastructure.

That being said, it was discussed at GHC, and we will bump the base version as soon as possible (but again, the intent is not to encourage maintainers to publish "tentative releases" to the primary index; the earliest time we encourage uploading packages targetting what currently is still "GHC HEAD" is when the first release-candidates of the respective GHC major release happen), this requires some technicalities to be addressed in GHC's build-system first, hence the delay.

RyanGlScott commented 6 years ago

Note that there is also a ghc-8.5-gross-hack branch that simply comments out the Divisible/Decidable instances for data types from StateVar and transformers. It's not a particularly nice solution, but it suffices if you just want to use contravariant itself (with slightly fewer bells and whistles) with GHC 8.5 in the meantime.

RyanGlScott commented 6 years ago

GHC HEAD now uses base-4.12 in HEAD, so we can begin patching transformers and StateVar (and the properly patch contravariant).

@svenpanne, do you wish to patch StateVar, or should I offer a patch?

svenpanne commented 6 years ago

Done in haskell-opengl/StateVar@ee33c10ab94127fad00c4dd1cee4096fe670b745 and haskell-opengl/StateVar@c55f6739e956412cfa97e11e6ba300a894b80013. I'm a bit unsure about the version bump for StateVar, though: Is the addition of the instance a major or minor version bump? Technically it's a major one, because one could have clashing instances now, but OTOH a major bump for the addition of an instance feels a bit odd...

RyanGlScott commented 6 years ago

Thanks! I'm currently working on getting the latest version of transformers (which similarly defines Contravariant instances) bundled with GHC, so once that's done, we'll no longer be blocked.

Is the addition of the instance a major or minor version bump?

From a PVP perspective, simply adding new instances does not require a major version bump.

phadej commented 6 years ago

@svenpanne I think we'll need to adjust released StateVar versions upper bounds on base to disallow base-4.12, so there won't be a instlal plans with "base-4.12 + some StateVarwithoutContravariant` instance. But this is something to check closer after the ghc branch is cut.

RyanGlScott commented 6 years ago

@phadej, I'm confused. Shouldn't that sort of thing be done on contravariant's side? After all, StateVar's bounds should be perfectly fine as they are—both old and new StateVar releases are buildable with the gamut of base versions.

phadej commented 6 years ago

@RyanGlScott one could have GHC-8.6 project using StateVar without ever depending on contravariant. It would be confusing if they somehow manage to pick up old StateVar version. (It's not even so far fetched: someone could use old Stackage snapshot and not bump StateVar version there).

See e.g. semigroups http://hackage.haskell.org/package/semigroups-0.18.4/semigroups.cabal For old GHC it doesn't even depend on text, containers etc. We trust that with recent GHCs Semigroup instances are defined in the respective packages.

RyanGlScott commented 6 years ago

I don't think I agree with your conclusion. If a GHC 8.6 user builds an old version of StateVar without depending on contravariant, so what? It'll still build just fine. If they are building contravariant, then its strict lower version bounds on StateVar (which I still need to put in place on #45) will ensure that they build a sufficiently new StateVar version that has Contravariant instances.

The only reason version bounds are used in the first place is to ensure that every combination of packages will build correctly, and StateVar already accomplishes that goal. Doing anything more feels overbearing.

RyanGlScott commented 6 years ago

And with http://git.haskell.org/ghc.git/commit/b41a42e3dc0c428344c553e195b7dc91272de21e, GHC HEAD now has everything it needs. I've just merged #45, so one can use master to build contravariant with GHC HEAD.

Remaining questions:

  1. Should I make a new contravariant release now? Or should I wait until it gets closer to time to release GHC 8.6, and use head.hackage in the meantime?
  2. How should we deal with old contravariant releases? I'm thinking that making revisions to ensure that they all have upper version bounds on base of < 4.12 would suffice, right?
phadej commented 6 years ago

I probably overthink this. Anyway, if there will be problems they can be fixed later.

RyanGlScott commented 6 years ago

Alright, here's what I've done:

  1. I've submitted a head.hackage patch for contravariant at https://github.com/hvr/head.hackage/pull/49.
  2. I've revised contravariant-1.4 and contravariant-1.4.1 (which, according to this, are the only two releases with build plans that would permit GHC HEAD) on Hackage, giving them base < 4.12.

I'll keep this issue open as a reminder to make a new release at some point before GHC 8.6 lands.

RyanGlScott commented 5 years ago

This was taken care of (a while back) with the release of contravariant-1.5.