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

ihatemoney: init at 4.1 plus module and test #68973

Merged
merged 5 commits into from
Jan 9, 2020
Merged

ihatemoney: init at 4.1 plus module and test #68973

merged 5 commits into from
Jan 9, 2020

Conversation

symphorien
Copy link
Member

Motivation for this change

missing package

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.

cc @Ekleog

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: module (update) This PR changes an existing module in `nixos/` labels Sep 17, 2019
FRidh
FRidh reviewed Sep 17, 2019
View reviewed changes
pkgs/development/python-modules/ihatemoney/default.nix Outdated
};
meta = {
homepage = "https://ihatemoney.org";
description = "A simple shared budget manager web application";
Copy link
Member

Choose a reason for hiding this comment

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

web application so it probably does not belong in python-packages.nix

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to use buildPythonApplication but using the wsgi.py needs to use this package as a python module and not a python application. Hence the placement in python-packages.nix.

nixos/modules/services/web-apps/ihatemoney/default.nix
group = "ihatemoney";
db = "ihatemoney";
python3 = config.services.uwsgi.package.python3;
pkg = python3.pkgs.ihatemoney;
Copy link
Member

Choose a reason for hiding this comment

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

here the package is selected

nixos/modules/services/web-apps/ihatemoney/default.nix
module = "wsgi:application";
chdir = "${pkg}/${pkg.pythonModule.sitePackages}/ihatemoney";
env = [ "IHATEMONEY_SETTINGS_FILE_PATH=${configFile}" ];
pythonPackages = self: [ self.ihatemoney ];
Copy link
Member

Choose a reason for hiding this comment

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

and here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but I know it is the same package because I use the python which was used to build uwsgi. This is precisely the reason to introduce the services.uwsgi.package internal option.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Sep 17, 2019
@Ekleog
Copy link
Member

Ekleog commented Sep 20, 2019

@FRidh The answers from @symphorien seem reasonable enough to me, do you think this needs more work or does this look good to you?

@symphorien
Copy link
Member Author

Any more comments?

FRidh
FRidh reviewed Oct 22, 2019
View reviewed changes
pkgs/development/python-modules/ihatemoney/default.nix Outdated
@@ -0,0 +1,61 @@
{ python3Packages, lib, fetchFromGitHub, nixosTests }:
Copy link
Member

Choose a reason for hiding this comment

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

Do not pass in python3Packages because it will be called for every python version. Instead, every dep needs to be a parameter of its own.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

aanderse
aanderse requested changes Nov 2, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

All nix code should be consistently indented with two spaces per level of indentation.

After these relatively trivial cosmetic nitpicks have been resolved I see no reason not to merge if @FRidh is satisfied.

pkgs/development/python-modules/ihatemoney/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/ihatemoney/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/ihatemoney/default.nix Outdated Show resolved Hide resolved
@symphorien symphorien force-pushed the ihatemoney branch 2 times, most recently from 09b3e67 to 32d2266 Compare November 2, 2019 12:56
@symphorien
Copy link
Member Author

I reindented the files.

@symphorien
Copy link
Member Author

Any interest in merging ?

Ekleog
Ekleog approved these changes Jan 5, 2020
View reviewed changes
nixos/tests/ihatemoney.nix Outdated
http = ":8000";
};
};
boot.cleanTmpDir = true;
Copy link
Member

Choose a reason for hiding this comment

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

Wanted to go ahead and merge, but there's just this remaining place with wrong indentation -- the below are just nits seen while re-reading through :)

nixos/tests/ihatemoney.nix Outdated Show resolved Hide resolved
nixos/tests/ihatemoney.nix Outdated Show resolved Hide resolved
symphorien and others added 3 commits January 5, 2020 22:04
@symphorien @Ekleog
Update nixos/tests/ihatemoney.nix
54b0ab0 
Co-Authored-By: Léo Gaspard <github@leo.gaspard.ninja>
@symphorien @Ekleog
Update nixos/tests/ihatemoney.nix
b8b9e7b 
Co-Authored-By: Léo Gaspard <github@leo.gaspard.ninja>
@symphorien
nixos/tests/ihatemoney.nix: run nixpkgs-fmt
665fee3 
now indentation is perfect
@symphorien
Copy link
Member Author

ran nixpkgs-fmt on nixos/tests/ihatemoney.nix.

@Ekleog
Copy link
Member

Ekleog commented Jan 9, 2020

@ofborg build nixosTests.ihatemoney

@Ekleog
Copy link
Member

Ekleog commented Jan 9, 2020

Thanks! Resolved the remaining conflicts, this LGTM, let's merge :)

@Ekleog Ekleog merged commit 8fcf992 into NixOS:master Jan 9, 2020
@symphorien symphorien deleted the ihatemoney branch March 21, 2020 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse aanderse requested changes

@FRidh FRidh FRidh left review comments

@Ekleog Ekleog Ekleog approved these changes

@infinisil infinisil Awaiting requested review from infinisil

@jonringer jonringer Awaiting requested review from jonringer

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 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 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@symphorien @Ekleog @FRidh @aanderse