Skip to content

Build nix packages from source #253

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

Open
wants to merge 11 commits into
base: hovudstraum
Choose a base branch
from

Conversation

toast003
Copy link
Contributor

Removed the u2c and partyfuse packages since the main copyparty package has them now.

This PR complies with the DCO; https://developercertificate.org/

@@ -73,25 +79,13 @@ let
++ lib.optional withBasicAudioMetadata mutagen
++ lib.optional withHashedPasswords argon2-cffi
++ lib.optional withZeroMQ pyzmq
++ (extraPythonPackages ps)
);
++ (extraPythonPackages python.packages);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I understand how this worked, so if someone could test it out I'll appreciate it

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems correct. We don't strictly need it anymore, since it was only used in the partyfuse package, but it's nice to have an escape hatch like this. Maybe @dtomvan can confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually that's still useful for using hooks with additional dependencies, so we keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

python.packages doesn't exist
I believe it's called python.pkgs?

Copy link
Contributor Author

@toast003 toast003 Jul 29, 2025

Choose a reason for hiding this comment

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

True, not sure where I got python.packages from, sorry 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I was planning to use it for hooks which might require other arbitrary dependencies, like plyer for notifications, but yes ps was originally just the python package set when you do python3.withPackages, so python.pkgs is equivalent here.

Usage is like this: copyparty.override { extraPythonPackages = (ps: with ps; [ plyer matplotlib ]); }

@chinponya
Copy link
Contributor

Thanks for working on this. It has been bothering me since I realized that the release source isn't just a copy of the repo and does actually contain the built frontend dependencies (which was the final argument for using sfx.py for the package). We never got around to changing it due to it being in the process of merging into nixpkgs, putting into question the future of our version of the package (I still don't know what's the right move here, but I digress).

Only a few things stand out to me:

  • partyfuse package had an additional dependency that's not present in the default copyparty package now (pyfuse), so it's broken. Perhaps it should be included by default.
  • The update script no longer supports using the local release file (and make assumptions about the future release url), but looking at the commit order during a release, that feature seems unused (right, @9001 ?). We might be better off removing it completely instead of trying to update it to work with release source archives.

@toast003
Copy link
Contributor Author

The update script no longer supports using the local release file (and make assumptions about the future release url), but looking at the commit order during a release, that feature seems unused (right, @9001 ?). We might be better off removing it completely instead of trying to update it to work with release source archives.

Yeah, I was thinking about that too, I just got sidetracked and forgot to comment on it here. If we want to keep that feature I think we could pull the version from somewhere in the tarball (copyparty/__version__.py maybe?)

partyfuse package had an additional dependency that's not present in the default copyparty package now (pyfuse), so it's broken. Perhaps it should be included by default.

Didn't notice, thanks for pointing that out!
I'll add it now

@chinponya
Copy link
Contributor

chinponya commented Jul 29, 2025

The update script no longer supports using the local release file (and make assumptions about the future release url), but looking at the commit order during a release, that feature seems unused (right, @9001 ?). We might be better off removing it completely instead of trying to update it to work with release source archives.

Yeah, I was thinking about that too, I just got sidetracked and forgot to comment on it here. If we want to keep that feature I think we could pull the version from somewhere in the tarball (copyparty/__version__.py maybe?)

That would work. If you want to add it, that's great, but feel free to ignore it, since it's an unused feature anyway. Personally I'm leaning towards just removing it, to simplify the update script.

partyfuse package had an additional dependency that's not present in the default copyparty package now (pyfuse), so it's broken. Perhaps it should be included by default.

Didn't notice, thanks for pointing that out! I'll add it now

Sorry, I lied, it's fusepy not pyfuse

@toast003
Copy link
Contributor Author

Ok, did both.

@toast003 toast003 marked this pull request as ready for review July 29, 2025 10:12
@chinponya
Copy link
Contributor

chinponya commented Jul 29, 2025

Okay, I've tried building and running it and we have some dependencies missing. They were a part of the sfx.py bundle hence why they were omitted in buildInputs.

I found a couple looking at pyproject.toml, setup.py and make-sfx.sh script. It might not be an exhaustive list:

  • pyftpdlib
  • partftpy
  • magic

@toast003
Copy link
Contributor Author

Ok, I'll add them in a bit.

Btw, how did you find out that we're missing dependencies? Haven't done extensive testing yet since I'm not too familiar with copyparty yet, but it does seem to run just fine as is?

@chinponya
Copy link
Contributor

chinponya commented Jul 29, 2025

These features are disabled by default but will hit import errors when enabled in the config.

@toast003
Copy link
Contributor Author

Ok, since they are optional should we lock them behind parameters like the other optional dependencies?

Also partytftp isn't packaged in nixpkgs, so that'll have to be packaged separately

@chinponya
Copy link
Contributor

Not surprising. That's @9001's fork of tftpy
https://github.com/9001/partftpy

Feature flags would be nice, but maybe keep pyftpdlib enabled by default to not break existing setups. magic is a recent addition and partftpy is for a very obscure feature, so keeping them off by default should be fine.

I've also noticed copyparty itself will provide details about optional dependencies when ran with --deps:

dependencies            found: sqlite (sessions and file/media indexing)
dependencies            found: pillow (image thumbnails (plenty fast))
dependencies            found: vips (image thumbnails (faster, eats more ram))
dependencies            found: pillow-webp (create thumbnails as webp files)
dependencies            found: ffmpeg (transcode audio, create spectrograms, video thumbnails, good-but-slow image thumbnails)
dependencies            found: ffprobe (transcode audio, create spectrograms, video thumbnails, read audio/media tags)
dependencies            found: mutagen (read audio tags (ffprobe is better but slower))
dependencies            found: argon2 (secure password hashing (advanced users only))
dependencies            found: pyzmq (send zeromq messages from event-hooks)
dependencies          missing: pillow-heif (read .heif images with pillow (rarely useful))
dependencies            found: pillow-avif (read .avif images with pillow (rarely useful))

Somehow we have pillow-avif despite never explicitly listing it as a dependency (it may have been an external dependency that got merged into pillow at some point?). pillow-heif might be worth including whenever withThumbnails is enabled.

I'd expect a lot more items on this list. @9001 Is this check just outdated/incomplete?

@toast003
Copy link
Contributor Author

From my (little) time looking at partyftpy I think we can get away with overriding the tftpy package to point to partyftpy's source code, so we don't need to package it right now.

Feature flags would be nice, but maybe keep pyftpdlib enabled by default to not break existing setups. magic is a recent addition and partftpy is for a very obscure feature, so keeping them off by default should be fine.

I'd say keep pyftpdlib on a default on flag instead, and the other two on default off flags

@toast003
Copy link
Contributor Author

toast003 commented Jul 29, 2025

For --deps being outdated, while I do think it's something we should address I would prefer if that happened on its own issue/pr

@dtomvan
Copy link
Contributor

dtomvan commented Jul 29, 2025

since I realized that the release source isn't just a copy of the repo and does actually contain the built frontend dependencies

Well, that makes it a whole lot easier, doesn't it? I initially was scared of migrating the package to a source build, because it looked like I needed to replicate the entire docker build process in nix. Luckily that's not the case. Thanks for this PR!

From my (little) time looking at partyftpy I think we can get away with overriding the tftpy package to point to partyftpy's source code, so we don't need to package it right now.

Well, I think they removed the setuptools build process here so it might not be possible. Chances are high if you specify hatch for the build system and set pyproject = true; it will work. I think it's best to do that without relying on the structure of nixpkgs' tftpy package though.

Also, great news, this actually improves startup performance due to the python files being stored uncompressed! Bench: (the 2nd copyparty is from the nix package at v1.18.4, it's outdated, I know, but it doesn't really matter here)

Benchmark 1: ./result/bin/copyparty --exit cfg
  Time (mean ± σ):     251.2 ms ±   7.5 ms    [User: 204.8 ms, System: 37.2 ms]
  Range (min … max):   233.5 ms … 260.8 ms    11 runs

Benchmark 2: copyparty --exit cfg
  Time (mean ± σ):     518.6 ms ±  13.4 ms    [User: 425.5 ms, System: 75.4 ms]
  Range (min … max):   496.2 ms … 546.6 ms    10 runs

Summary
  ./result/bin/copyparty --exit cfg ran
    2.06 ± 0.08 times faster than copyparty --exit cfg

Copy link
Contributor

@dtomvan dtomvan left a comment

Choose a reason for hiding this comment

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

LGTM after partytftp gets added

mainProgram = "copyparty";
sourceProvenance = [ lib.sourceTypes.binaryBytecode ];
sourceProvenance = [ lib.sourceTypes.fromSource ];
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way this is implicitly the default when no sourceProvenance was specified.

@toast003
Copy link
Contributor Author

toast003 commented Jul 29, 2025

Well, I think they removed the setuptools build process here so it might not be possible. Chances are high if you specify hatch for the build system and set pyproject = true; it will work. I think it's best to do that without relying on the structure of nixpkgs' tftpy package though.

I didn't check github when I was adding partftpy so I ended up doing what I said, and it seems to work just fine.
Edit: pyproject.toml still has set setuptools as the build backend

That said, I'm not sure what you mean with not relying on the structure of nixpkgs?

@chinponya
Copy link
Contributor

chinponya commented Jul 29, 2025

That said, I'm not sure what you mean with not relying on the structure of nixpkgs?

As in, we don't want any potential changes to the tftpy package in nixpkgs to break our partftpy package, so it's best to include a complete package definition for it instead of overriding tftpy.

@chinponya
Copy link
Contributor

Ok, I ran the current version and see no issues. I even deployed it on my big instance and nothing broke, with no changes necessary to my config. It's not a big sample size but it's better than nothing.
It looks ready to merge (unless you still want to change something).
I hope all this back and forth discussion wasn't too discouraging for you.

@toast003
Copy link
Contributor Author

toast003 commented Jul 29, 2025

It looks ready to merge (unless you still want to change something).

Nah I don't want to change anything for now, I was thinking of trying to build the frontend deps but I don't want to deal with that rn. Maybe some other time.

I hope all this back and forth discussion wasn't too discouraging for you.

Don't worry about it, y'all are pretty nice ^^

@chinponya
Copy link
Contributor

@9001 👀

"version": "1.18.7",
"hash": "sha256-GRoiNnYRWNcq5ITywPv7bK4pdgTey6Or1cZqyE2XDf4="
"hash": "sha256-Zzj2I5BSduhmS9h5HxSX0MYfiBbGYI52xNkJehKEkXA="
Copy link
Contributor

Choose a reason for hiding this comment

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

New copyparty version released since then (again), so this will need to be bumped before (or after) merging.
You could wait until @9001 finds the time to review and merge it, to not play a catch-up game with every bugfix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This particular bugfix doesn't matter for the PR since we don't use the sfx anymore, but yeah, I'll wait for @9001 to review it

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the bugfix is for "older versions of python3", and well, nixpkgs always keeps their default python3 fairly recent, and get rid of EOL versions, so this is a non-issue to us.

Also, to make review easier, I think it's completely fine to squash some stuff upfront. Like 3 would be fine I think: (1) remove u2c and partyfuse, (2) build from source, (3) fixup dependencies. I think that makes it more straightforward for ed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, squashed.

Btw sorry for messing up the commit history, had a bit of a brain fart 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Lol that's fine. Honestly I like the transparency it sometimes provides while one is responding to review suggestions. You can see the thought process. But yeah in most cases succinctness is more important IMO.

But enough bikeshedding 😅

toast003 added 2 commits July 31, 2025 17:50
nix: remove u2c and partyfuse packages
The main copyparty package has u2c and partyfuse, so these packages are
redundant now

nix: add fusepy dependency

fix: nix:  use replace pyfuse with fusepy
@toast003
Copy link
Contributor Author

toast003 commented Jul 31, 2025

Turns out the code that 48dfd73 removed is used by a script :)
Don't have the brainpower to deal with this rn, I'll mark the pr as a draft, and fix it tomorrow

@toast003 toast003 marked this pull request as draft July 31, 2025 23:46
Comment on lines +79 to +81
src = fetchurl {
inherit (pinData) url hash;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Just saying this is something that can be done, it's quite common for python packages in nixpkgs and I figured why not...

Suggested change
src = fetchurl {
inherit (pinData) url hash;
};
src = fetchPypi {
inherit (pinData) pname version hash;
};

I think this also allows us to move away from the custom update script, because nix-update has special support for PyPi packages. That's for another PR I think though.

Then all the info would be contained in the derivation, like this:

# no pin.json anymore
buildPythonApplication rec {
  pname = "copyparty";
  version = "1.18.8";
  src = fetchPyPi {
    inherit pname version;
    hash = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";

And the update script would then be a measly nix-update copyparty --flake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I do think it would be a good idea, if we were to do this @9001 would need to have nix installed to be able to update the nix packages (assuming that nix-update depends on nix), so we would need to know if he's ok with that.
If he is I'll be glad to do this in a separate PR tho

Copy link
Contributor

@dtomvan dtomvan Aug 2, 2025

Choose a reason for hiding this comment

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

Alternatively (maybe digging the hole even further in the process lol) you could have a Github action with on: workflow_dispatch that does the update with nix-update I guess...

assuming that nix-update depends on nix
Yeah, it needs nix to evaluate stuff. That's about the only advantage of the home-grown py+json solution right now: it doesn't involve any nix files... but at the same time that's a decoupling that usually isn't really desirable

TBF the updater for openjdk in nixpkgs also does this...

Revert "nix: remove unused code from update.py"

This reverts commit 48dfd73.

nix: update local release pin function in update.py
@toast003 toast003 marked this pull request as ready for review August 1, 2025 12:12
@justinas
Copy link

justinas commented Aug 1, 2025

Very new to this project and mostly just curious. Given this now builds from source and not build artifacts, what is the point of the pin, i.e. why isn't the source of the package just set roughly to ../../..?

Currently this approach results in counter-intuitive behavior: if you do nix run github:9001/copyparty, i.e. try to run the latest commit (implicit), it runs the pinned stable release. What's more, if you attempt to "run" a specific version, i.e. nix run github:9001/copyparty/v1.18.9, it will build and run v1.18.8 instead (as that was the pin as of v1.18.9 release). Not to mention that nix run in a local checkout will not use local sources, and thus is unsuitable for development.

@toast003
Copy link
Contributor Author

toast003 commented Aug 2, 2025 via email

@justinas
Copy link

justinas commented Aug 2, 2025

Thanks, I see. I missed the fact that this still builds from somewhat "pre-processed" source code, not the literal contents of the repo. I understand why that is, as the build process for the frontend assets seems complex and quite ad-hoc.

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

Successfully merging this pull request may close these issues.

4 participants