chore: vendor hasql - #5084
chore: vendor hasql#5084
Conversation
Pipelining mode
As a workaround for Hackage including the dependencies of internal libs in the public list of package dependencies.
* master: Update actions
* clean-up-public-deps: Remove rerebase from the internal library deps
Report errors when LISTEN or UNLISTEN fail
Remove stack from repo since I am currently working only with cabal
|
(let's only discuss the MIT-copyright case here, since Diogo already agreed to contribute hasql-notifications via MIT license anyway)
Sure, this is what this PR is doing essentially. Maybe in different words, but it's the same. The whole discussion is about Steve not wanting to change that LICENSE file. And then it becomes a problem. You can not do the following:
In this case you are violating copyright, because clearly you imply that the copyright in It gets worse, though: Even if you argue "yeah, but technically it doesn't say where to reproduce the copyright", you will suddenly have code in Now someone who consumes the PostgREST repository comes along, takes code from This means that the copyright notices for the vendored code and our PostgREST code must be in the same place, as you already pointed out. |
But it can be a different top level file - wouldn't that solve the issue? (I am not really sure if we are not talking past each other really).
I don't think it implies that but I agree it might look like that. The whole point is that we don't have to be specific when saying "PostgREST includes large portion of hasql" and include required copyright notice somewhere. It does not matter where - the only thing that matters is that it is included both in source and binary distributions (and in the repository). MIT license is not file based: you can copy/paste parts of the licensed code into your own code and it is fine as long as you include required copyright notice somewhere. It is in contrast to eg. MPL, which is file scoped.
It is easy: just state somewhere clearly that PostgREST includes third-party libraries and what their license terms are. It is on users to familiarize themselves with license terms of all the software they use in their derivative works - not on us.
Looks like indeed - that might be the easiest thing to do: maintain a file with a list of dependencies, their copyright notices and possibly links to their licenses. |
d2f1597 to
1f00775
Compare
|
Both @diogob and @nikita-volkov have agreed via email to contribute the code we're vendoring here to the PostgREST project under the MIT license with the current "The PostgREST contributors" copyright. Thank you a ton, both of you - this makes it much easier for us! 🚀 ❤️ The relevant commit is 6702809, which removes the vendored licenses and thus makes the top-level license take effect. Would appreciate an approval from both of you for public transparency. Your approval will only be counted as "OK with the license/copyright change", not necessarily as an approval of forking/vendoring in general. We'll technically also need an approval from @laurenceisla, who contributed diogob/hasql-notifications@962ae98 to |
The LICENSE file is now unchanged.
laurenceisla
left a comment
laurenceisla
left a comment
There was a problem hiding this comment.
We'll technically also need an approval from @laurenceisla
Approving as stated above 👍
|
I approve this change. You have my permission to merge the copyright notice into "The PostgREST contributors", with the git history preserving my authorship. This approval is strictly for the copyright adjustment. It is not an endorsement of the vendoring decision. The PostgREST team assumes full responsibility for the maintenance of this forked version. |
https://github.com/nikita-volkov/hasql-pool/tree/v1.3.0.5 contains an important fix to pool capacity handling. BTW. How are we going to handle upstream fixes? Do they need to be ported manually? Secondly - sorry for late chipping in here, but putting all separate libraries into a single source tree blurs API boundaries between them and is anti-modular. (I am not sure if it also makes backporting of upstream changes difficult). |
This removes everything from the vendored hasql packages, except their license, source code, tests and .cabal file. We also remove metadata which is not needed anymore from .cabal file, but leave everything that needs to be ported to our own .cabal file later in place.
Both Nikita (hasql, hasql-pool, hasql-dynamic-statements, hasql-implicits) and Diogo (hasql-notifications) have agreed to contribute these projects at the respective version to the PostgREST project under the MIT license and under the "The PostgREST contributors" copyright our license has. This makes it significantly easier for us to ensure we're complying with their copyright and our downstream users will, too. A big thank you to both of them.
To run it, we need to remove the Arrows extension from postgrest.cabal. It doesn't seem to be depended upon by the hasql code (but was still introduced along with it), but breaks the style check. That's because stylish-haskell applies all extensions found in the .cabal file for all files, even when they are only part of *one* component. The Arrows extension makes `proc` a reserved keyword, which is used in CallPlan.hs.
I did not fix all of these by hand, but used the `--refactor` flag on hlint to do it for me. This is something that we might want to add in the future by default.
1f00775 to
5aca0a3
Compare
|
Just rebased to be able to merge fast-forward, no changes. Edit: Sorry, @mkleczek, did not read your comment until after the merge. Will respond to it later. |
IIRC, this was in a code path that we do not use, correct? This would be code that would be removed in the next step anyway - we don't need to keep our vendored version of hasql generally useful, we can restrict it to the code we are actually using to reduce maintenance effort.
It depends on what you mean by "ported". You probably can not just take them as is, and try to apply them here, because: So essentially, you'd have to acknowledge the problem and then write your own fix (ideally without looking at the upstream change!).
The latter is not important, because see above. Also backporting will already not work well, because we apply our own formatters and linters - cherry-picks will quickly not apply anymore anyway! The idea is to reduce the code to the minimum code we use - which means we are using everything in that codebase. IMHO, the biggest benefit of having the hasql code modular is, that the user can pick which parts of the library they want to use and which they don't need. This is a non-issue for us, because everything that we have in here is used. |
At the moment -yes. But let's not hand-wave away the problem of maintenance and bugfixes backporting.
I wouldn't remove this code because it is useful and potentially very relevant to us (ie. running some
That is going to cost us a lot in terms of effort and - what's worse - quality, I'm afraid.
It sounds like you are saying "modularity is not needed if you are not publishing modules separately" - which is clearly backwards. Modularity is needed to keep software maintainable - does not matter if modules are public or private. |
14 participants
Step 2 for #4823. This replaces the
hasql-*dependencies (which includeshasql-notifications, but I don't know of a way not to fork that as well, since it depends onhasqlitself) with a vendored copy keeping the original history.We're only using the source code, so far, but not the original tests. That will be a next, separate step to merge these tests with ours and integrate them into the nix tooling etc.
Right now, it builds. Let's see what CI says - I assume it should also pass all our tests.
TODO:
Depends on #5093 and #5094.