junixsocket{,-native}-common: init at 2.10.1#427538
junixsocket{,-native}-common: init at 2.10.1#427538SuperSandro2000 merged 2 commits intoNixOS:masterfrom
Conversation
|
The new packages LGTM, but I don't know about maven packaging. And do we have to unconditionally link them into postgresql_jdbc and keycloak, or can users add these manually to nixos configurations such as |
The rationale behind linking them to In keycloak, we have the problem that it does not use the But apart from that detail, the same rationale is applied here: Yes, we could leave junixsocket out, so everyone would have to add it via the plugins themselves. But I can't think of any sane setup (given that keycloak is a security-relevant application!) where you would run keycloak's database on a different machine than keycloak itself, and hence where you would want to connect it to its database via TCP/IP when you could connect it directly via Unix sockets. But, both integrations into |
|
@Infinidoge @NickCao Do you have any decision on that topic? Should I reduce my PR, or should we keep the out-of-the-box unix socket support for all postgresql-jdbc consumers and keycloak. |
I'd say let's leave the out-of-the-box unix socket support for later. |
e6caa36 to
4e8b7b9
Compare
@NickCao Done. Please review again. |
|
|
There was a problem hiding this comment.
Looks good!
I ran nixpkgs-review on both aarch64-darwin and aarch64-linux and posted the results above. I also ran:
nix build .#junixsocket-common .#junixsocket-native-common
sha1sum result*/share/java/*
and verified the SHA-1 sums match Maven Central.
I did not build or run anything dependent on the JARs (but as they are new, we know nothing will be broken by them -- nixpkgs-review also verified this.)
|
One more nitpick: for some reason the |
Is this because |
Yeah, but I expected |
|
I made another pr (PR #433781) based on this PR because I was inspired by the brevity of top-level @vog: you might want to try the same modification, see: https://github.com/NixOS/nixpkgs/compare/cb87cf22932de66cdb347958037d05f0c5bdafe6..2e37e33388ef62b367448285bb97fd559e9709cd |
What are your reservations?
I don't really know which is the best way. But I don't see any packages in nixpkgs that are doing it that way, nor can I find any documentation that definitively says what is the best thing to do in this case. If you think you can make it work and get approved by people with more experience and authority than me, go for it. I'm curious what that would look like, so if you think it can be done I'd say it's worth doing just as an experiment. |
This simply doesn't make any sense to me. It is cumbersome and wasteful. fetchMavenArtifact already creates a derivation that contains exactly the files we want, in the structure we want, with the naming we want. Just the metadata is missing. But instead of improving that function to just pass down the missing metadata, more code is written, to create another derivation, into which all binary files are copied, so now they are twice in the store, at least until the next "gc" or "optimize" action. And all that extra machinery just because the original derivation is missing an attribute. To me, that current way requires justification. Why the hell people are doing that? It is cumbersome and wasteful. Do I overlook something?
Yes, that's what I found confusing, too. But the missing metadata explains this at least to some level.
Indeed, I'll go ahead and try to fix that in another PR, then let's see if this is acceptable, or if anyone raises valid concerns that I may have overlooked. |
Done! See PR #433975. |
I'm just looking to learn the current state-of-the-Nix-art, you're looking to push it forward. Awesome! (Eventually, of course, I would like to see more of these Maven artifacts being built from source, but having a simple mechanism to bring in binaries is nice, too -- and an important stepping stone.) I'll take a look at PR #433975. |
4e8b7b9 to
1838087
Compare
Ok, it's done! PR #433975 has been accepted, a I rebased this one onto the latest master. @msgilligan @NickCao Is there anything else to be done for this PR? |
|
|
msgilligan
left a comment
There was a problem hiding this comment.
A minor request: add a unique description to junixsocket-native-common.
1838087 to
72427e2
Compare
@msgilligan Ok, done! Please review again. |
|
|
|
Successfully created backport PR for |
Okay! Since junixsocket has been accepted by now, I've created the other two PRs building on that: |
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.