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

2 package stregsystemet #3

Closed
wants to merge 15 commits into from
Closed

2 package stregsystemet #3

wants to merge 15 commits into from

Conversation

Mast3rwaf1z
Copy link
Member

options seem to be better suited for a seperate issue

@Mast3rwaf1z Mast3rwaf1z linked an issue Oct 26, 2023 that may be closed by this pull request
6 tasks
Copy link
Contributor

@AshesOfEther AshesOfEther left a comment

Choose a reason for hiding this comment

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

Here's your review! Should ask before this is merged, has Stregsystemet been shown to successfully run when built from this?

@@ -0,0 +1,34 @@
{pkgs ? import <nixpkgs> {}, lib ? pkgs.lib}:
Copy link
Contributor

Choose a reason for hiding this comment

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

It's bad practice to not pin Nixpkgs, as the version on one system might not match that on another.

What we should do is create a flake.nix that imports Nixpkgs and uses pkgs.callPacakge to provide the packages in each file. This would also make ? pkgs.lib redundant.

Suggested change
{pkgs ? import <nixpkgs> {}, lib ? pkgs.lib}:
{pkgs, lib}:

Comment on lines 2 to 4
pkgs ? import <nixpkgs> {},
fetchFromGitHub ? pkgs.fetchFromGitHub,
lib ? pkgs.lib
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Suggested change
pkgs ? import <nixpkgs> {},
fetchFromGitHub ? pkgs.fetchFromGitHub,
lib ? pkgs.lib
pkgs,
fetchFromGitHub,
lib

fetchFromGitHub ? pkgs.fetchFromGitHub,
lib ? pkgs.lib
}:
with lib;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary, and it works fine without. It's also poor practice, as it can make it hard to tell when something from lib is being used.

Suggested change
with lib;

Comment on lines 8 to 10
django-select2 = pkgs: pkgs.callPackage ./development/django-select2.nix {};
env = (pkgs.python3.withPackages (pythonPackages: with pythonPackages; [
(django-select2 pkgs)
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have pkgs, no need to have it passed in via a function.

Suggested change
django-select2 = pkgs: pkgs.callPackage ./development/django-select2.nix {};
env = (pkgs.python3.withPackages (pythonPackages: with pythonPackages; [
(django-select2 pkgs)
django-select2 = pkgs.callPackage ./development/django-select2.nix {};
env = (pkgs.python3.withPackages (pythonPackages: with pythonPackages; [
django-select2

Comment on lines 22 to 54
pkgs.stdenv.mkDerivation rec {
pname = "stregsystemet";
version = "2.4.5";

src = pkgs.fetchFromGitHub {
owner = "f-klubben";
repo = "stregsystemet";
rev = "v${version}";
sha256 = "sha256-dd3TnyH7+iFH1rZFOZcWw+66Jh4S6DeU67cPTwJIJLY=";
};

installPhase = ''
mkdir -p $out/bin
mkdir -p $out/usr/share/stregsystemet
cat > local.cfg << EOF
[general]

[debug]

[database]

[hostnames]
1=192.168.122.202
EOF
sed -i '1 i #!${env.interpreter}' manage.py

cp ./* $out/usr/share/stregsystemet -r
ln -s $out/usr/share/stregsystemet/manage.py $out/bin/stregsystemet
#mkdir -p /opt/stregsystemet
#touch /opt/stregsystemet/data.json
#touch /opt/stregsystemet/db.sqlite3
'';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be doable more cleanly with buildPythonApplication. I'm certainly a bit weary of the non-standard use of /usr/share inside a derivation.

@Mast3rwaf1z
Copy link
Member Author

@TobiasSN sure i'll look at the suggestions and test it within a few days

Mast3rwaf1z and others added 5 commits March 4, 2024 15:18
Contains a basic NixOS setup, as well as other basic scaffolding.

Co-authored-by: Tobias SN <[email protected]>
Contains a basic NixOS setup, as well as other basic scaffolding.

Co-authored-by: Tobias SN <[email protected]>
@Mast3rwaf1z Mast3rwaf1z closed this Mar 5, 2024
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.

Package Stregsystemet
2 participants