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

sturmflut: init at 0-unstable-2023-04-25 #316460

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

zebreus
Copy link
Contributor

@zebreus zebreus commented Jun 1, 2024

Description of changes

sturmflut is a pixelflut client written in C.

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.

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Thanks for this, @zebreus, much appreciated, please find below a few suggestions for improvement.

I'd appreciate it if the new package is also run through nixfmt 🙂

pkgs/by-name/st/sturmflut/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/st/sturmflut/package.nix Outdated Show resolved Hide resolved
@afh
Copy link
Member

afh commented Jun 3, 2024

I see platforms was recently changed from unix (which includes darwin) to linux, well done 👍 as the package unfortunately does not build on darwin as:

  • it hardcodes CC to gcc which can be mitigated using makeFlags = [ "CC=${stdenv.cc.targetPrefix}cc" ];
  • but then errors with sys/sendfile.h not being found

Might be nice to raise awareness about those issues upstream if there is interest in supporting darwin.

@zebreus
Copy link
Contributor Author

zebreus commented Jun 3, 2024

@afh Thanks for the review ❤️. I applied your suggestions and formatted the file with nixfmt.

I was thinking about opening some upstream PRs anyway, as I also have some issues with sturmflut not being able to load some of my images. Also it would be been nice to have a tagged release of sturmflut.

@zebreus zebreus requested a review from afh June 3, 2024 11:26
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 labels Jun 3, 2024
@afh
Copy link
Member

afh commented Jun 3, 2024

You're very welcome, @zebreus 🙂, and thanks for considering and accepting my comments and suggestions.

Feel free to ping me in the darwin related upstream issues or here if there is progress on that effort, happy to help.

Indeed tagged release would be much appreciated.

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

LGTM, although being on aarch64-darwin I'm not able to verify/test this package locally, let's see what ofborg and the checks have to say…

@zebreus
Copy link
Contributor Author

zebreus commented Jun 3, 2024

@afh I submitted an upstream patch (TobleMiner/sturmflut#13) for darwin support, but am not really able to test it as I don't have a darwin device.

I also put a version of this PR that builds the patched sturmflut at https://github.com/zebreus/nixpkgs/tree/test-sturmflut-darwin . Can you try if it compiles and works on your machine?

nix build github:zebreus/nixpkgs/test-sturmflut-darwin#sturmflut --print-build-logs

@afh
Copy link
Member

afh commented Jun 3, 2024

Thank you, @zebreus, for the extra effort to put up a separate branch for me to test and for including instructions how to test it; very, very helpful and very, very much appreciated! 💯

The changes you propose fix the darwin build 🎉
Happy to help test further if that would be helpful, although being unfamiliar with sturmflut and pixelflut I'd appreciate some instructions on how to do so…

nix build github:zebreus/nixpkgs/test-sturmflut-darwin#sturmflut --print-build-logs
source> trying https://github.com/zebreus/sturmflut/archive/292bb5b1c5153acc45bfd32f17374e9731cd8ca5.tar.gz
source>   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
source>                                  Dload  Upload   Total   Spent    Left  Speed
source>   0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
source> 100  8511    0  8511    0     0  10902      0 --:--:-- --:--:-- --:--:-- 29147
source> unpacking source archive /private/tmp/nix-build-source.drv-0/292bb5b1c5153acc45bfd32f17374e9731cd8ca5.tar.gz
sturmflut> Running phase: unpackPhase
sturmflut> unpacking source archive /nix/store/fbjh60jglwbnlggnfr0fgd793yimbnrx-source
sturmflut> source root is source
sturmflut> Running phase: patchPhase
sturmflut> Running phase: updateAutotoolsGnuConfigScriptsPhase
sturmflut> Running phase: configurePhase
sturmflut> no configure script, doing nothing
sturmflut> Running phase: buildPhase
sturmflut> build flags: SHELL=/nix/store/60qp4q78hlg1fsvq4np6iv0gpqrl4v4p-bash-5.2p26/bin/bash
sturmflut> rm -f sturmflut
sturmflut> clang -Ofast -Wall -march=native -pthread -DIMAGICK=7 image.c network.c main.c progress.c -lpthread `pkg-config --cflags --libs MagickWand` -o sturmflut
sturmflut> Running phase: installPhase
sturmflut> install flags: SHELL=/nix/store/60qp4q78hlg1fsvq4np6iv0gpqrl4v4p-bash-5.2p26/bin/bash DESTDIR=\$\(out\) install
sturmflut> install -Dm755 sturmflut /nix/store/p39zy15r7hh1s20hi6rff896smvrp3gx-sturmflut-0-unstable-2024-06-03/bin/sturmflut
sturmflut> Running phase: fixupPhase
sturmflut> checking for references to /private/tmp/nix-build-sturmflut-0-unstable-2024-06-03.drv-0/ in /nix/store/p39zy15r7hh1s20hi6rff896smvrp3gx-sturmflut-0-unstable-2024-06-03...
sturmflut> patching script interpreter paths in /nix/store/p39zy15r7hh1s20hi6rff896smvrp3gx-sturmflut-0-unstable-2024-06-03
sturmflut> stripping (with command strip and flags -S) in  /nix/store/p39zy15r7hh1s20hi6rff896smvrp3gx-sturmflut-0-unstable-2024-06-03/bin
% ./result/bin/sturmflut
Please specify a host to send to
USAGE: ./result/bin/sturmflut <host> [file to send] [-p <port>] [-a <source ip address>] [-i <0|1>] [-t <number of threads>] [-m] [-o <offset-spec>] [-O] [-s <percentage>] [-S] [-h]

@afh
Copy link
Member

afh commented Jun 4, 2024

Loving the latest changes on this package, @zebres!! 👍

@JohnRTitor JohnRTitor changed the title sturmflut: init sturmflut: init at 0-unstable-2023-04-25 Jul 12, 2024
@JohnRTitor
Copy link
Contributor

Result of nixpkgs-review pr 316460 run on x86_64-linux 1

1 package built:
  • sturmflut

Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

LGTM

@JohnRTitor JohnRTitor merged commit 15173ed into NixOS:master Jul 12, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 10.rebuild-linux: 1 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.

3 participants