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

ihatemoney: init at 4.1 plus module and test #68973

Merged
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

};
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.

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

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.

@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?

@@ -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

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 ?

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
Co-Authored-By: Léo Gaspard <github@leo.gaspard.ninja>
Co-Authored-By: Léo Gaspard <github@leo.gaspard.ninja>
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants