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

sharkey wip (dont merge yet) #296697

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

CutestNekoAqua
Copy link
Contributor

@CutestNekoAqua CutestNekoAqua commented Mar 17, 2024

Description of changes

WIP PR to show the progress of Sharkey > Nixpkgs
continuation of #259829

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Mar 17, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Mar 17, 2024
@CutestNekoAqua
Copy link
Contributor Author

noo ofborg 💀

I forgot to swap out the maintainer info there for a second before pushing 😭

@CutestNekoAqua
Copy link
Contributor Author

Info for people looking at this:
sharkey does not build on machines with 4GB or RAM or less. :/

@CutestNekoAqua
Copy link
Contributor Author

CutestNekoAqua commented Mar 17, 2024

@Weathercold can you test this for me and give me your thoughts on it? This way we can get it merged quicker*

*limiting factor will still be getting someone with commit rights to merge in the end ;P

Edit: pinging you here because your question on the old PR makes me assume you are interested in using it once its merged.

@pluiedev
Copy link
Contributor

noo ofborg 💀

I forgot to swap out the maintainer info there for a second before pushing 😭

I was about to ask why was I pinged for this 😹 I guess this is tangentially related though since the pnpm deps code is from Vesktop

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 17, 2024
@CutestNekoAqua
Copy link
Contributor Author

noo ofborg 💀
I forgot to swap out the maintainer info there for a second before pushing 😭

I was about to ask why was I pinged for this 😹 I guess this is tangentially related though since the pnpm deps code is from Vesktop

yes. this is a unholy fusion of vesktop and #161855

maybe I will do that for misskey too later down the line.. Vesktop is the first pnpm nix impl that works here tho 😸

@pluiedev
Copy link
Contributor

pluiedev commented Mar 17, 2024

Have you seen #290715? That is essentially Vesktop's pnpm deps logic packaged into a fetcher 😺

@pluiedev
Copy link
Contributor

pluiedev commented Mar 17, 2024

Also, please mark the PR as a draft if you don't want it merged or thoroughly reviewed by other people with write perms :) Change the title too to the standard format: 'sharkey: init at x.y.z'

@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Mar 17, 2024
@CutestNekoAqua
Copy link
Contributor Author

Have you seen #290715? That is essentially Vesktop's pnpm deps logic packaged into a fetcher 😺

yes, but I would not like to wait for it :d

@CutestNekoAqua
Copy link
Contributor Author

Also, please mark the PR as a draft if you don't want it merged or thoroughly reviewed by other people with write perms :) Change the title too to the standard format: 'sharkey: init at x.y.z'

I want it thoroughly reviewed tho! Its not marked as a draft because it should be tested already! I just dont want it to be merged because my commits dont follow the contribution guidelines right now and I will forcepush to make the changes into commits how they should be once we tested everything <3

@CutestNekoAqua
Copy link
Contributor Author

I am fairly certain, that this code already 100% works and would be ready to merge, but because we did not setup a server with it yet to test if everything including AP federation works flawlessly, its not ready to merge in my eyes.

Copy link
Contributor

@pluiedev pluiedev left a comment

Choose a reason for hiding this comment

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

It's a bunch of nitpicking but honestly it looks pretty good — now we just have to figure out what needs to be done next

{ config, ... }:

{
services.misskey = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Still says misskey here

ensureDatabases = [ "sharkey" ];
ensureUsers = [
{
name = "sharkey;
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing closing quotation mark

-        name = "sharkey;
+        name = "sharkey”;

owner = "TransFem-org";
repo = "Sharkey";
domain = "activitypub.software";
rev = "${finalAttrs.version}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: unnecessary quotes & interpolation

postPatch = ''
mv package.json package.json.orig
jq --raw-output ". * $pnpmPatch" package.json.orig > package.json
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

ime running pnpm with --force(as in #290715) seems to be more reliable than this package.json patching in some cases involving cross-platform builds? not sure on the conditions of such a thing happening, so take with a pinch of salt

Copy link
Member

Choose a reason for hiding this comment

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

--force is better, yes. See pnpm/pnpm#7695

But I am still not very confident on this fixed-output derivation as it does not provide stable hashes across different versions of pnpm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, how can we execute scripts without making it unreproducible?

@sodiboo
Copy link
Contributor

sodiboo commented May 26, 2024

Hey, is this still being worked on? I'm interested in hosting an instance of Sharkey on NixOS, and this seems like a good start probably. If this is abandoned, i might try to continue these efforts in a new PR.

@CutestNekoAqua
Copy link
Contributor Author

Hey, is this still being worked on? I'm interested in hosting an instance of Sharkey on NixOS, and this seems like a good start probably. If this is abandoned, i might try to continue these efforts in a new PR.

its not abandoned. other things in my calendar where just taking up time more. Feel free to request changes / add yourself as second maintainer later.

@sodiboo
Copy link
Contributor

sodiboo commented May 28, 2024

I based some work in my own config off of this PR. I've got it running now. The modules are in the sharkey/ subdirectory of my config (permalink) with my own configuration at ../sharkey.mod.nix relative to the link. it's running live at gaysex.cloud.

@CutestNekoAqua
Copy link
Contributor Author

awesome! i will look update this PR in a day or two


(
cd node_modules/.pnpm/node_modules/v-code-diff
node scripts/postinstall.js
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is named postinstall.cjs, and it needs to have the correct extension to run.

It seems clearer to just run their postinstall script, which does actually just run this file.

Suggested change
node scripts/postinstall.js
pnpm run postinstall

preBuild = ''
export HOME=$(mktemp -d)
export STORE_PATH=$(mktemp -d)
export NODE_OPTIONS="--max_old_space_size=4096"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? The frontend fails to build with a very unintuitive error, which on some sources say that this flag will fix because it's out of memory. Manually running cd packages/frontend; pnpm build (so that it actually shows its output rather than being mangled by the TTY-centric "overview" interface) shows that it's actually v-code-diff failing to build, as far as i can tell. Running that install script correctly makes this flag unnecessary for me.

jemalloc
ffmpeg-headless
stdenv.cc.cc.lib
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to add a binary path for nodePackages.pnpm and bash, because they are used in the npm scripts of Sharkey (e.g. pnpm migrateandstart will use shell scripting && to run pnpm migrate && pnpm start; in particular, pnpm isn't qualified here so it needs to be in PATH, even if we qualify it fully for direct invocations)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
]);
]);
binPath = lib.makeBinPath [
bash
nodePackages.pnpm
];


makeWrapper ${nodePackages.pnpm}/bin/pnpm $out/bin/sharkey \
--run $out/data
--prefix LD_LIBRARY_PATH : ${libPath} \
Copy link
Contributor

Choose a reason for hiding this comment

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

See above comment at the start of the install phase.

Suggested change
--prefix LD_LIBRARY_PATH : ${libPath} \
--prefix LD_LIBRARY_PATH : ${libPath} \
--prefix PATH : ${binPath} \

mkdir -p $out/data/packages/client
ln -s /var/lib/sharkey $out/data/files
ln -s /run/sharkey $out/data/.config
cp -r locales node_modules built fluent-emojis tossface-emojis sharkey-assets packages package.json pnpm-workspace.yaml $out/data
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these files are nonexistent. Why are we selecting them in such a fragile way?

Suggested change
cp -r locales node_modules built fluent-emojis tossface-emojis sharkey-assets packages package.json pnpm-workspace.yaml $out/data
cp -r * $out/data

''
runHook preInstall

mkdir -p $out/data
Copy link
Contributor

Choose a reason for hiding this comment

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

Why data? That feels very generic and undescriptive. Why not, e.g., Sharkey (matching the actual name of the corresponding directory in a typical install)

# https://gist.github.com/MikaelFangel/2c36f7fd07ca50fac5a3255fa1992d1a

makeWrapper ${nodePackages.pnpm}/bin/pnpm $out/bin/sharkey \
--run $out/data
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a backslash.

Suggested change
--run $out/data
--run $out/data \

makeWrapper ${nodePackages.pnpm}/bin/pnpm $out/bin/sharkey \
--run $out/data
--prefix LD_LIBRARY_PATH : ${libPath} \
--prefix NODE_ENV : production
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also missing a backslash. Also, why --prefix? This isn't a list.

Suggested change
--prefix NODE_ENV : production
--set-default NODE_ENV production \

--run $out/data
--prefix LD_LIBRARY_PATH : ${libPath} \
--prefix NODE_ENV : production
--add-flags migrateandstart
Copy link
Contributor

@sodiboo sodiboo May 28, 2024

Choose a reason for hiding this comment

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

Should this really be always passed? Wouldn't it make more sense to not be passing it and let the consumer choose which script to invoke? e.g. a consumer might want to run just sharkey migrate when doing internal maintenance, or just sharkey start to start without a db migration. there also exists sharkey init which is mostly useless because sharkey migrate will initialize an empty DB, and there's also sharkey revert which undoes a migration, and that one is not covered by migrateandstart.

migrateandstart is a good choice for the "default command", though, so we should absolutely be using that for the systemd service.

'';

postBuild = ''
pnpm build --reporter=ndjson
Copy link
Contributor

Choose a reason for hiding this comment

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

This --reporter=ndjson doesn't seem to do much, because pnpm build here is just a script that immediately invokes pnpm -r build and --reported=ndjson does not get passed to it. Also, why is this postBuild? That's clearly the main build step.

${pkgs.envsubst}/bin/envsubst -i "${configFile}" > /run/sharkey/default.yml
cd ${pkgs.sharkey}/data
'';
serviceConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this running as root?

Suggested change
serviceConfig = {
serviceConfig = {
User = "sharkey";

SystemCallFilter = "@system-service";
UMask = "0077";
};
environment.NODE_ENV = "production";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we setting NODE_ENV here and in the wrapper?

(
cd node_modules/.pnpm/node_modules/v-code-diff
node scripts/postinstall.js
)
Copy link
Contributor

Choose a reason for hiding this comment

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

re2 and sharp also need install scripts to run. They use node-gyp, so we need to set the node path or else it'll try to fetch headers from nodejs.org.

Without them, the build succeeds but we get runtime errors as the binaries can't be loaded (so, the package won't actually work).

Suggested change
)
)
export npm_config_nodedir=${nodePackages.nodejs}
(
cd node_modules/.pnpm/node_modules/re2
pnpm run rebuild
)
(
cd node_modules/.pnpm/node_modules/sharp
pnpm run install
)

@h7x4 h7x4 added 8.has: documentation 8.has: module (new) This PR adds a module in `nixos/` labels Jun 1, 2024
@SuperSandro2000 SuperSandro2000 marked this pull request as draft June 6, 2024 11:46
@foxyseta
Copy link

its not abandoned. other things in my calendar where just taking up time more. Feel free to request changes / add yourself as second maintainer later.

I am a Nix novice, but I'd be happy to help, too, if you are still very busy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants