Skip to content
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 #342893

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

winterqt
Copy link
Member

@winterqt winterqt commented Sep 18, 2024

Description of changes

This supersedes the package portion of #274052, as discussed in Matrix, by packaging Draupnir in a way that is:

  • future-proof (won't break when they move to Yarn v4),
  • more correct (patches better-sqlite3 to link against our SQLite3, with thanks to @lf- for the inspiration),
  • and easier to maintain (doesn't use mkYarnPackage, thus requires slightly less work, like no manual wrapping being required).

However, this has the caveat that it requires a vendored package-lock.json. Sorry!

This needs testers! I'm not sure if I can just drop in the Mjölnir module and test it or not, though I guess someone can just test it with the module from #274052.

Thanks to @TheArcaneBrony for creating the original PRs and the module, which will continue to be worked on in that PR.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@winterqt winterqt requested a review from mweinelt September 18, 2024 22:31

# TODO: move to preNpmRebuild
preBuild = ''
rm -r node_modules/better-sqlite3/{build,deps/common.gypi,deps/sqlite3.gyp,deps/sqlite3}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# Remove things we know we don't need that just increase closure size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that still needed for sqlite?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the removed files are all only used by the better-sqlite3 package to build it's own vendored sqlite. better-sqlite3-dynamic-link.patch replaces that with just linking against the nixpkgs sqlite.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

nativeBuildInputs = [ pkg-config ];
buildInputs = [ sqlite ];

# TODO: move to preNpmRebuild
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure yet if I'd like to do this in this PR (would decrease build time by a bit), but we'll see after it gets tested.

uses across an entire homeserver.
'';
license = licenses.afl3;
maintainers = with maintainers; [ winter ];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheArcaneBrony Do let me know if you'd like to be added as a maintainer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, sounds good

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC vendored package-lock.json was why the Draupnir package was rewritten from scratch

@olivia-fl
Copy link
Contributor

@winterqt

I'm not sure if I can just drop in the Mjölnir module and test it or not

Using this package directly with the mjolnir module isn't quite drop-in because the mjolnir module doesn't have a package option. Just setting mjolnir = draupnir in an overlay doesn't work because there are a few places that the mjolnir module hardcodes things that changed:

  • ${pkgs.mjolnir}/bin/mjolnir path
  • --mjolnir-config option
  • the default config path at ${pkgs.mjolnir}/libexec/mjolnir/deps/mjolnir/config/default.yaml

The mjolnir default config is compatible with draupnir, so we ended up just doing

systemd.services.mjolnir.serviceConfig.ExecStart = lib.mkForce ''${pkgs.draupnir}/bin/draupnir --draupnir-config ./config/default.yaml'';

With this hack, we're running the package on @scd31's infrastructure. It appears to be working so far. Will see if anything breaks and come back with more details.

@winterqt
Copy link
Member Author

Thank you!

@winterqt
Copy link
Member Author

I may try to write a proper NixOS test for this some time this weekend for the hell of it.

@lf-
Copy link
Member

lf- commented Nov 19, 2024

hmmmmmmmm. can we use https://github.com/imsnif/synp or similar to generate the package.lock from the yarn.lock automatically during the build step so we don't have to check it in? it appears it just needs to exist here so we don't have to use yarn right?

nvm this looks suspiciously not possible. but maybe it can still be done by other means??

@winterqt
Copy link
Member Author

we could but i think it'd be way too fragile dependency tree resolving wise (e.g. yarn may produce a diff. tree that npm would need to call out to the internet to fix etc.)

@lf-
Copy link
Member

lf- commented Nov 19, 2024

we could but i think it'd be way too fragile dependency tree resolving wise (e.g. yarn may produce a diff. tree that npm would need to call out to the internet to fix etc.)

ideally we could convince npm to just accept that it got a different solution than it would have dreamt up itself, but yeah it is a little sketchy. relatable problems with dealing with flakes so i get it. just wish there was a better way somehow.

@winterqt
Copy link
Member Author

dynamic drvs will save us all :(

@TheArcaneBrony
Copy link
Contributor

Worst case you can use the draupnir module from my package and specify the package option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants