-
-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draupnir: init at 2.0.0-beta.6 #274052
base: master
Are you sure you want to change the base?
Draupnir: init at 2.0.0-beta.6 #274052
Conversation
Result of 2 packages blacklisted:
1 package built:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a lot of potential to improve the module and to bring it in line with today's standards.
I can see that it is a copy of the mjolnir module, but I'm not willing to accept a new module of similar quality, because it is hard to maintain.
This is especially relevant with RFC42 (settings & formatters) in mind
Time for the almighty rebase on nixos:master... Please don't close the PR again... |
28ee30d
to
a0c9de0
Compare
🎊 It didn't close as merged! |
a0c9de0
to
cd3ce15
Compare
(Renamed the package init commit) |
cd3ce15
to
38a2e8f
Compare
(^ master rebase) |
Co-authored-by: teutat3s <10206665+teutat3s@users.noreply.github.com>
^ just renamed the maintainers commit and added the co author to the tests pr :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the intention behind moving homeserverUrl
? This now needs changing in the docs to fix the manual.
tests = { | ||
inherit (nixosTests) draupnir; | ||
}; | ||
updateScript = ./update.sh; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything special here, that nix-update-script
couldn't handle? It can also deal with updating the Yarn cache hash in the derivation.
updateScript = ./update.sh; | |
updateScript = nix-update-script { }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hashes are stored in a separate file, I think the reason was that nix-update-script couldn't handle the Yarn cache hash? I'd have to test this once a new Draupnir version comes out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question, does anyone know how to make better-sqlite3 work under nix? Seems adding sqlite
as nativeBuildInput doesn't work, and this causes the roomStateBackingStore
feature of Draupnir to cause a startup failure as better-sqlite3 fails to find it's bindings file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
share/draupnir/node_modules/better-sqlite3/
share/draupnir/node_modules/better-sqlite3/src/better_sqlite3.cpp
share/draupnir/node_modules/better-sqlite3/src/better_sqlite3.hpp
Looks like it is here, but does not get build. Ideally we can build it outside of draupnir, as a dedicated package, and patch it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, from earlier testing, seems to also be possible to rebuild it with a single command, and when I raised possibly making it a sub-package I was recommended not to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff
diff --git a/pkgs/by-name/dr/draupnir/package.nix b/pkgs/by-name/dr/draupnir/package.nix
index fb2faad56350..81a8ee568fbd 100644
--- a/pkgs/by-name/dr/draupnir/package.nix
+++ b/pkgs/by-name/dr/draupnir/package.nix
@@ -3,7 +3,10 @@
, makeWrapper
, nodejs
, matrix-sdk-crypto-nodejs
+, python3
, sqlite
+, srcOnly
+, removeReferencesTo
, mkYarnPackage
, fetchYarnDeps
, nixosTests
@@ -12,6 +15,7 @@
# docs: https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/javascript.section.md#yarn2nix-javascript-yarn2nix
let
hashesFile = builtins.fromJSON (builtins.readFile ./hashes.json);
+ nodeSources = srcOnly nodejs;
in
mkYarnPackage rec {
pname = "draupnir";
@@ -37,11 +41,30 @@ in
yarnLock = src + "/yarn.lock";
packageJSON = ./package.json;
+ pkgConfig = {
+ "@matrix-org/matrix-sdk-crypto-nodejs" = {
+ postInstall = ''
+ # replace with the built package
+ cd ..
+ rm -r matrix-sdk-crypto-nodejs
+ ln -s ${matrix-sdk-crypto-nodejs}/lib/node_modules/@matrix-org/* ./
+ '';
+ };
+
+ better-sqlite3 = {
+ nativeBuildInputs = [ python3 ];
+ postInstall = ''
+ # build native sqlite bindings
+ npm run build-release --offline --nodedir="${nodeSources}"
+ find build -type f -exec \
+ ${removeReferencesTo}/bin/remove-references-to \
+ -t "${nodeSources}" {} \;
+ '';
+ };
+ };
+
#prebuild phase
preBuild = ''
- # copy built modules to package...
- echo "Copying built matrix-sdk-crypto-nodejs modules to package..."
- cp -a ${matrix-sdk-crypto-nodejs}/lib/node_modules/* node_modules/
echo "Adding version.txt..."
mkdir -p deps/draupnir/
echo "${version}-nix" > deps/draupnir/version.txt
matrix-appservice-discord also seems to bundle it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to mention, I applied your patch in bd7179d
Fixes build with corepack upstream introduced by the-draupnir-project/Draupnir#472 Upstream may move to yarn v4, need to investigate options. (the-draupnir-project/Draupnir#475) Looks like the recommended option is to use a fixed-output derivation? Sources: - https://www.reddit.com/r/NixOS/comments/1f8tq94/its_2024_has_anyone_figured_out_how_to_build_a/ - https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/networking/cluster/tilt/assets.nix
mkdir -p deps/draupnir/ | ||
echo "${version}-nix" > deps/draupnir/version.txt | ||
|
||
sed -i 's/corepack //g' deps/draupnir/package.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Copy paste from commit description)
fixup! draupnir: init at 2.0.0-beta.6 - ugly sed hack to drop corepack
Fixes build with corepack upstream introduced by the-draupnir-project/Draupnir#472
Upstream may move to yarn v4, need to investigate options. (the-draupnir-project/Draupnir#475)
Looks like the recommended option is to use a fixed-output derivation?
Sources:
- https://www.reddit.com/r/NixOS/comments/1f8tq94/its_2024_has_anyone_figured_out_how_to_build_a/
- https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/networking/cluster/tilt/assets.nix
Any suggestions? I'm sure this is an ugly, unnecessary hack, but I couldn't find any documentation on the topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheArcaneBrony This already uses a fixed-output derivation(the offlineCache
part), AFAICT if upstream moves to Yarn v4, this package will either need to convert the lockfile to the old version like pgadmin4 or wait until fetchYarnDeps
to get v2 lockfile support.
…x-sdk-crypto-nodejs and better-sqlite3 Co-Authored-By: div72 <div72@localhost>
Description of changes
NOTE: This PR is a successor of #222939. The previous PR was closed by accident during a rebase operation.
Original description:
Draupnir is a hardfork of Mjolnir. Mjolnir package has been unmaintained due to upstream bugs. This package is mostly a drop-in replacement. This package also uses newer methods of handling dependencies.
Repository can be found at:
https://github.com/the-draupnir-project/Draupnir
Note on testing: package and module were (and still are) tested in our production environment, works fine as far as it's been used.
Note on replacing Mjolnir: Not sure whether this is appropriate, due to general usage of the bot changing. You can however fully automatically migrate from mjolnir to draupnir.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.