build(nix): refactor nix stuff completely #1096
savyajha
nex
Labels
No labels
Bug
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status/Blocked
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
To-Merge
Wont fix
old/ci/cd
old/rust
No project
No assignees
6 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!1096
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "Aviac/continuwuity:flake-parts-isation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
We keep the old code in the nix subdirectory for good meassure.
Currently, this allows users to build:
This doesn't include all the cross compilation builds so
they didn't work beforehand anyways so 🤷 This also gave me the change to simplify a lot of the code massively. I'm sure we can add back cross compilation once there is a bit more of demand. For now this here should give us a new good base to work with.
Verification:
The flake includes tests for both variants of the build as well as nix native checks for a bunch of things like: rustfmt, taplo fmt, nextest, doctest, audits, licenses ...
I think with the move of the hashes it's now a good idea to look into running https://github.com/Mic92/nix-update/ in CI as a replacement of my workflow.
Is there a particular reason to keep the old code around? It'll still be in the git history
Yeah ok, makes sense. I just used it while developing to mirror the code for convenience. I'll remove it 👍
@Shuroii I tried the following:
and got the following diff
I'm very confused ^^'
The path holds teh rocksdb source code.
A few minor nits and questions (I'm not familiar with crane, so cannot comment there), but more around rocksdb and liburing.
@ -0,0 +10,4 @@# own. In order for this to work, we need to set flags on the build that match# whatever flags tikv-jemalloc-sys was going to use. These are dependent on# which features we enable in tikv-jemalloc-sys.packages.rust-jemalloc-sys' =You can just use
packages.rust-jemalloc-sys-unprefixedfrom upstream nixpkgs, which should work properly. If using the upstream package, might not be worth it to disable c++ or docs.My first try naively replacing the package ran into this here:
I'll try further
This usually happens when multiple versions of jemalloc are being used. Can you also check what
ldd <final binary without checkPhase>gives you?This comes from the
jemalloc_proffeature flag being enabled somehow even though it's in the disabled features list ... I'll look into itIdeally the output should look like so:
libjemalloc should be fairly high up, ideally above libc. We suffered from a similar problem while upgrading from rc6 -> rc7 in nixpkgs.
Ok found it ...
jemalloc_profgets enabled by thefullfeature set which was enabled on accident!Just for the record:
jemalloc_profenabled the config option pair mentioned in this lineUgg, the warning actually went away but it was unrelated as you thought :(
Could this be related?
It might, it might not. The bottom line is that the linking is not happening properly. Was the package building before your changes? I'd recommend removing the customisations you have made to jemalloc and liburing and building after that.
Idk, I can't get it to work and I don't want to spend more of my sunday on it for now. It seems like the cxx upstream feature just creates some kind of trouble ... otherwise it wouldn't succeed without it. I tried a lot of variants now but I'm getting tired of waiting 2min for the compilation :p
I'll come back once I get some motivation again. Maybe we can still merge this in the mean time and fix it some other day?
Yes, with no problem. It also passes the NixOS test
My personal guess is that in the unprefixed version there is a clash between the cpp and rust symbols. I don't know how it works upstream then though
@ -0,0 +6,4 @@...}:{packages.liburing = pkgs.liburing.overrideAttrs (prev: {Why not just use upstream to reduce build time?
It's honestly not the part that takes longest in the build so I didn't really care yet ^^ I think this takes like sub 10 sec
@ -0,0 +9,4 @@{packages = {rocksdbBase =(pkgs.rocksdb_9_10.override {pkgs.rocksdb.overridewill also work.@ -0,0 +13,4 @@# Override the liburing input for the build with our own so# we have it built with the library flagliburing = self'.packages.liburing;jemalloc = self'.packages.rust-jemalloc-sys';jemalloc = pkgs.rust-jemalloc-sys-unprefixedif you're okay using upstream.@ -0,0 +27,4 @@cmakeFlags =lib.subtractLists [# No real reason to have snappy or zlib, no one uses this"-DWITH_SNAPPY=1"Ideally use
(lib.cmakeBool "WITH_SNAPPY" true)instead of-DWITH_SNAPPY=1, imo. That's nixpkgs best practices.@ -0,0 +65,4 @@# Unsetting this so we don't have to revert it and make this nix exclusivepatches = [ ];postPatch = ''We don't need these, the upstream package handles these well enough.
Unfortunately it was broken when I tried to remove them. I will try again though with the rest of your suggestions!
I guess those patches from upstream in postPatch should also be completely disabled since we did them in our fork
Basically you can just omit
postPatchaltogether. UpstreampostPatchis fine. However, keeppatchesempty.I think what might be happening here is that upstreams
(lib.versionOlder finalAttrs.version "7")is confused by us overwriting the version to something not semver, hence it executes the optional stuff which is non-sense for our version
The thing is, that
follydoesnt exist in our rocksdb version:https://forgejo.ellis.link/continuwuation/rocksdb/src/branch/10.5.fb/third-party
I'm confused, upstream rocksdb also doesn't have it in tree. Do we need to fetch this? How does upstream nix handle this?
https://github.com/facebook/rocksdb/tree/main/third-party
argggg. I set
version = "v10.5.fb"when it should've beenversion = "10.5.fb"._.(this way
lib.versionOlderreturns the correct result)Anyways, this is solved then. Thanks again!
@ -0,0 +75,4 @@'';});rocksdb =Instead of creating multiple rocksdb variant packages, why not create a single one where you can override liburing/jemalloc using bools? That's how it's done upstream. I think it might be a cleaner solution and reduce a few loc.
@savyajha Thanks for the review! That's exactly what was missing, very good stuff and I think this simplified a few things really nicely 💯
I'd recommend just using unchanged upstream
rust-jemalloc-sys-unprefixedandliburingto avoid unnecessary recompilations here. You don't need to redefine the packages, just use the nixpkgs versions. The changes being made aren't major.I haven't reviewed the jemalloc changes, but I'm not aware of anything that would prevent upstream. Liburing should auready be upstream? We nornally use distro packages for that iirc.
libarg in one file dfea251318fullto the list of default disabled features b2c7dd57d1vprefix) 9c48e55093@ -0,0 +63,4 @@doCheck = true;nativeBuildInputs = [# bindgen needs the build platform's libclang. Apparently due to "splicingShould this not be
self'.rocksdb?It's a bit complex. This is a function that's called in other functions like
buildPackageandbuildDeps(below). These functions are called innix/packages/continuwuity/default.nixwith the right version of rocksdb depending on the feature set, like so:@savyajha Any further thoughts?
@Aviac I'd still prefer it if we used upstream rust-jemalloc-sys - it seems to work perfectly in nixpkgs as well as in the previous iteration. I would like to go through this and debug this thoroughly but I've got too much on my plate IRL atm. Apart from that I think this is a very well-thought out implementation and I have learned a lot going through your code. :)
Are we good to merge in this state then?
@savyajha wrote in #1096 (comment):
Yeah me too. Can we do that as a follow up? Are there any open issues from your side aside from that?
No other comments from me. Looks good to go!
Needs a rebase before we merge because of the lockfile conflict
ee47a5d7abf715f16773Sorry, I also pushed some new changes on the branch that I did after the rebase. It includes
treefmta formatter for the whole tree in nix. I will put them in a separate branch though so we can have it in a separate PR.b3df1fca11c24862aa6d@Jade now it's properly rebased. I made one small addition though, which is the last commit. It adds
hydrajobs which define jobs for the nix build server softwarehydra. I'm going to build it with my own build server so that we're constantly going to verify that everything on the nix side works.@Aviac feel free to mention me or request a review when you're ready for merge
I think the treefmt being part of this is fine, and maybe making a formatting check too? I think some of the commits should be squashed as well :3
c24862aa6d4631546e7a4631546e7abc3db4c04d@nex I'm basically done
@nyanbinary just pinging you FYI
3ea2aa54e63791f26d0a1c27e08fbdf27d378c32f27d378c32a5bece9389a5bece93892dc05b4e7f2dc05b4e7f0da967f962@savyajha @nyanbinary It seems we were stuck on a bad hash. After doing a
nix flake updatewe can use upstreamrust-jemalloc-sys-unprefixed🎉0af23c404fe6a0885688@nex @Jade this is in pretty good shape now. Should I squash something a little bit more? Any other general requests before merge?
admin media delete-past-remote-media#1136admin media delete-past-remote-media#1136I think the last remaining comment is do we want to format all the toml in continuwuation/continuwuity@!1096 (commit
5957c71b2d) ? Expecially without adding to precommit and CIView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.