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.

Sign up for GitHub

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

Jump to bottom

gotify-server: init at 2.0.10 #70441

Merged
merged 2 commits into from Oct 25, 2019
Merged

gotify-server: init at 2.0.10 #70441

merged 2 commits into from Oct 25, 2019

Conversation

doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 5, 2019 •
edited

Motivation for this change

Fixes #67384.

@Lassulus, to complement #69869, here's a working derivation of gotify server. It's the first time I've built a derivation which depends upon a yarn build and packr. I've based most of it upon riot-desktop.nix which is an electron app but uses yarn2nix-moretea.

The only thing I couldn't find a nicer way to do it, is making servers/gotify/default.nix require servers/gotify/ui.nix without adding gotify-server-ui = callPackage ../servers/gotify/ui.nix { }; to all-packages.nix.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@doronbehar doronbehar mentioned this pull request Oct 5, 2019
Closed
10 tasks
@Ma27 Ma27 self-requested a review October 5, 2019 09:48
@Ma27 Ma27 mentioned this pull request Oct 5, 2019
Closed
Ma27
Ma27 reviewed Oct 5, 2019
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Just played around with this a bit. Apart from the fact that it says unknown@unknown when requesting the application's version, this seems fine to me 👍

However it should be discussed if we want to add even more derivations with such a huge diff to nixpkgs - but I don't have a strong opinion about this :)

@doronbehar
Copy link
Contributor Author

I'm happy to know this fixes #67384 ☺️ . I'm aware of the version issue. According to upstream's build instructions, We need to use the following ldflags:

-w -s -X main.Version=$(git describe --tags | cut -c 2-) -X main.BuildDate=$(date "+%F-%T") -X main.Commit=$(git rev-parse --verify HEAD)  -X main.Mode=prod

I've set in the current build main.Version=${version} and main.Mode=prod. Should we add the rest? I'm not sure what -w -s mean but the commit hash is not easily retrievable since we use a release tarball and the BuildDate should always be 1970 so what difference would that make?

@Ma27 did you try to run upstream's compiled version of gotify and got a different response for the version? I wouldn't mind working on that so we'll emulate better upstream's behavior while maintaining the executable reproducible.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/71

@Ma27 Ma27 self-assigned this Oct 25, 2019
@Ma27
Copy link
Member

Ma27 commented Oct 25, 2019

Just pushed a commit which hopefully fixes eval. Currently working on a module/test for this and fixed the version issue locally :)

@ofborg ofborg bot requested a review from kalbasit October 25, 2019 10:11
@doronbehar
Copy link
Contributor Author

@Ma27 perhaps could you share your fix / module? I've updated the PR to 2.0.10 and pushed a fix for the ui package needed in all-packages.nix along with yours.

@doronbehar doronbehar changed the title gotify-server: init at 2.0.8 gotify-server: init at 2.0.10 Oct 25, 2019
@doronbehar
Copy link
Contributor Author

Looks good to me.

@Ma27
Copy link
Member

Ma27 commented Oct 25, 2019

@ofborg test gotify-server

@Ma27
Copy link
Member

Ma27 commented Oct 25, 2019

@ofborg test gotify-server

@Ma27 Ma27 merged commit 6db4ae1 into NixOS:master Oct 25, 2019
@doronbehar
Copy link
Contributor Author

doronbehar commented Oct 25, 2019 via email

infinisil
infinisil reviewed Oct 29, 2019
View reviewed changes
nixos/modules/services/web-apps/gotify-server.nix
default = "gotify-server";
description = ''
The name of the directory below <filename>/var/lib</filename> where
gotify stores its runtime data.
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this was made configurable?

@doronbehar doronbehar mentioned this pull request Nov 8, 2019
Closed
@doronbehar doronbehar deleted the package-gotify-server branch March 2, 2023 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infinisil infinisil infinisil left review comments

@Ma27 Ma27 Ma27 approved these changes

@kalbasit kalbasit Awaiting requested review from kalbasit

Assignees

@Ma27 Ma27

Labels
6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Package request: gotify
4 participants
@doronbehar @nixos-discourse @Ma27 @infinisil