RFC: allow forked dependencies, drop hackage release #2422
Comments
|
Overall I agree 👍
To reduce maintenance, would it be worth to drop stack support? We tried to do this before(#2271) but we couldn't make the cabal build work for Windows. For FreeBSD I recall it worked fine. |
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
added
the
idea
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:
|
|
Ah, that's certainly much better, I didn't understand it as "forking in the PostgREST org". |
This version of hasql-pool is a simplified rewrite that doesn't use the resource-pool package. The major API changes are that idle connections are no longer timed out (and the corresponding setting is gone), and that `release` makes the pool unusable, where it used to remain usable and only flushed idle connections. We depend on a PostgREST fork of 0.7.2 that gives us reliable flushing, compare PostgREST/hasql-pool#1 - hasql-pool 0.7 removes timing out of idle connections, so this change removes the db-pool-timeout option. Given that we were typically running with very high timeout settings, I don't anticipate the lack of timeout to introduce new issues, though we might want to consider introducing some retry-logic down the line when we encounter connection failures. - See PostgREST#2422 for a discussion on depending on a forked dependency. Besides adding the dependency to the nix overlay, we're also adding it to stack.yaml and a new cabal.project to allow stack/cabal users to build the project.
This version of hasql-pool is a simplified rewrite that doesn't use the resource-pool package. The major API changes are that idle connections are no longer timed out (and the corresponding setting is gone), and that `release` makes the pool unusable, where it used to remain usable and only flushed idle connections. We depend on a PostgREST fork of 0.7.2 that gives us reliable flushing, compare PostgREST/hasql-pool#1 - hasql-pool 0.7 removes timing out of idle connections, so this change removes the db-pool-timeout option. Given that we were typically running with very high timeout settings, I don't anticipate the lack of timeout to introduce new issues, though we might want to consider introducing some retry-logic down the line when we encounter connection failures. - See PostgREST#2422 for a discussion on depending on a forked dependency. Besides adding the dependency to the nix overlay, we're also adding it to stack.yaml and a new cabal.project to allow stack/cabal users to build the project.
This version of hasql-pool is a simplified rewrite that doesn't use the resource-pool package. The major API changes are that idle connections are no longer timed out (and the corresponding setting is gone), and that `release` makes the pool unusable, where it used to remain usable and only flushed idle connections. We depend on a PostgREST fork of 0.7.2 that gives us reliable flushing, compare PostgREST/hasql-pool#1 - hasql-pool 0.7 removes timing out of idle connections, so this change removes the db-pool-timeout option. Given that we were typically running with very high timeout settings, I don't anticipate the lack of timeout to introduce new issues, though we might want to consider introducing some retry-logic down the line when we encounter connection failures. - See PostgREST#2422 for a discussion on depending on a forked dependency. Besides adding the dependency to the nix overlay, we're also adding it to stack.yaml and a new cabal.project to allow stack/cabal users to build the project.
This version of hasql-pool is a simplified rewrite that doesn't use the resource-pool package. The major API changes are that idle connections are no longer timed out (and the corresponding setting is gone), and that `release` makes the pool unusable, where it used to remain usable and only flushed idle connections. We depend on a PostgREST fork of 0.7.2 that gives us reliable flushing, compare PostgREST/hasql-pool#1 - hasql-pool 0.7 removes timing out of idle connections, so this change removes the db-pool-timeout option. Given that we were typically running with very high timeout settings, I don't anticipate the lack of timeout to introduce new issues, though we might want to consider introducing some retry-logic down the line when we encounter connection failures. - See #2422 for a discussion on depending on a forked dependency. Besides adding the dependency to the nix overlay, we're also adding it to stack.yaml and a new cabal.project to allow stack/cabal users to build the project.
|
@robx Now that haskellari/postgresql-libpq#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? |
|
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. |
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:
nix/overlays/haskell-packages.nix(and additionally in a newcabal.projectandstack.yaml, to allow building without nix to keep working); we don't rename the forked package or upload it to hackageDownsides I'm aware of:
(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.)
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.)
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. #2391 (
hasql-pool) and #2349 (hasqlandpostgresql-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.
The text was updated successfully, but these errors were encountered: