postgrest icon indicating copy to clipboard operation
postgrest copied to clipboard

RFC: allow forked dependencies, drop hackage release

Open robx opened this issue 3 years ago • 4 comments

I'd like to suggest that we drop the implicit requirement that all of our Haskell dependencies are published hackage releases, primarily to allow us to move a bit more quickly when changes require patching libraries we depend on. I'm filing this issue to invite discussion -- maybe there are some stronger arguments against this than I'm aware of, or there's a better solution to the underlying problem.

What this would mean concretely is:

  • when a change requires a patched version of an upstream dependency, we make the change in a forked git repository, and we configure that in nix/overlays/haskell-packages.nix (and additionally in a new cabal.project and stack.yaml, to allow building without nix to keep working); we don't rename the forked package or upload it to hackage
  • we stop publishing PostgREST itself to hackage

Downsides I'm aware of:

  • PostgREST doesn't automatically make it into distributions that do blanket hackage imports (e.g. NixOS). (I don't see this as a big issue considering e.g. the automatic PostgREST package in NixOS has been broken for a while. This might rather improve the situation there, and I think there's enough interest in PostgREST for distributions to actively want to package it.)
  • We'd have less pressure to upstream patches to dependencies, resulting in less incentive to make high-quality changes, and less incentive to be good contributors to the general Haskell ecosystem. E.g., if we're building say against our own fork of hasql, it'll be much easier to merge a low-quality quick fix to that, rather than go to the effort of submitting a high quality pull request to upstream and engaging upstream maintainers in discussing the change. (We might try to address this by documenting that we aim to stay close to upstream, and to submit our changes upstream, until it becomes clear that we're diverging too far at which point making a proper fork that we publish to hackage would become reasonable.)
  • There's the general technical overhead of maintaining that nix overlay, stack.yaml etc. and the forked repos themselves.

The aim of this suggestion is to allow us to move a bit more freely when some changes we want to make require changes to upstream libraries. If we're convinced a dependency change is good for PostgREST, we can merge and release that, instead of having to wait for upstream to merge our change (or find a different solution). (This would also relieve some pressure on upstream maintainers, knowing they can take their time without holding us back.)

Concrete context is some changes I've been making which are currently waiting on upstream, e.g. https://github.com/PostgREST/postgrest/pull/2391 (hasql-pool) and https://github.com/PostgREST/postgrest/pull/2349 (hasql and postgresql-libpq).

(I don't mean to criticize the work of upstream maintainers here, I know myself that it's hard work, particularly when it's about supporting use cases that you're not personally interested in.)

Let me know if you think this is an awful idea, and/or if have alternative suggestions! Else I'd aim to go ahead with this sooner or later with one of the PRs that are currently stalled.

robx avatar Aug 11 '22 14:08 robx

Overall I agree :+1:

There's the general technical overhead of maintaining that nix overlay, stack.yaml etc. and the forked repos themselves.

To reduce maintenance, would it be worth to drop stack support? We tried to do this before(https://github.com/PostgREST/postgrest/issues/2271) but we couldn't make the cabal build work for Windows. For FreeBSD I recall it worked fine.

steve-chavez avatar Aug 11 '22 15:08 steve-chavez

We'd have less pressure to upstream patches to dependencies, resulting in less incentive to make high-quality changes, and less incentive to be good contributors to the general Haskell ecosystem.

I think that's a serious issue, because it effectively means that we're writing code that is not properly reviewed or tested.

Code that is in our own repo is held to quality standards by reviews and automated testing.

Upstream code is held to quality standards by the respective maintainers, including their tests etc.

Code in forked repos is none of that. It's hidden.

It's not only about actual quality of code - but also about perceived quality. Having important parts hidden that way would make me feel uneasy.

wolfgangwalther avatar Aug 11 '22 16:08 wolfgangwalther

We'd have less pressure to upstream patches to dependencies, resulting in less incentive to make high-quality changes, and less incentive to be good contributors to the general Haskell ecosystem.

I think that's a serious issue, because it effectively means that we're writing code that is not properly reviewed or tested.

Code that is in our own repo is held to quality standards by reviews and automated testing.

Upstream code is held to quality standards by the respective maintainers, including their tests etc.

Code in forked repos is none of that. It's hidden.

Why do you think the forked code wouldn't be visible or reviewed? Bringing in a change from a forked library would involve a review of that change.

Maybe it should be spelled out more concretely, but I see us:

  • making a fork of a repo in the PostgREST org
  • depending on the main branch of that repo
  • filing PRs against that repo, and having a regular review process

robx avatar Aug 12 '22 08:08 robx

Ah, that's certainly much better, I didn't understand it as "forking in the PostgREST org".

wolfgangwalther avatar Aug 12 '22 09:08 wolfgangwalther

@robx Now that https://github.com/haskellari/postgresql-libpq/pull/47 has been merged for quite a while and been released in https://github.com/haskellari/postgresql-libpq/releases/tag/v0.10.0.0, can we get rid of https://github.com/PostgREST/postgresql-libpq? Or is there anything else in there that we still need?

I can prepare a PR if we don't need the fork anymore.

I assume we would put the repo in archive mode, because it's still referenced by older commits in this repo?

wolfgangwalther avatar Jan 21 '24 10:01 wolfgangwalther

Given that we have #2275 open and currently no forks in use, I will close this ticket. The goal should be to only have temporary forks max, so that we can always release to hackage.

wolfgangwalther avatar Jan 25 '24 20:01 wolfgangwalther