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

riot-web: make package configurable #36265

Merged
merged 1 commit into from Mar 4, 2018
Merged

riot-web: make package configurable #36265

merged 1 commit into from Mar 4, 2018

Conversation

Ekleog
Copy link
Member

@Ekleog Ekleog commented Mar 3, 2018

Make riot-web more easy to configure for eg. a NixOS install: having a configured riot-web vhost becomes a matter of:

{
  services.nginx.virtualHosts."riot.example.org" = {
    enableACME = true; forceSSL = true;
    root = pkgs.riot-web.override {
      config = ''
        { the config.json contents }
      '';
    };
  };
}

cc @bachp

@Ekleog
Copy link
Member Author

Ekleog commented Mar 3, 2018 •
edited

Actually I've just been reminded that I could just locations."/config.json".alias = pkgs.writeText ...;.

But well, the person who told me that having also told me they first started to write the same PR as me then found this trick somewhere, I guess that's the “natural” way we expected to do stuff. So I'm leaving this open, despite the fact another solution exists :)

Mic92
Mic92 reviewed Mar 4, 2018
View reviewed changes
pkgs/applications/networking/instant-messengers/riot/riot-web.nix Outdated
@@ -12,6 +13,7 @@ stdenv.mkDerivation rec {
installPhase = ''
mkdir -p $out/
cp -R . $out/
${lib.optionalString (config != null) "cp ${configFile} $out/config.json"}
Copy link
Member

Choose a reason for hiding this comment

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

ln -s ?

@Ekleog
Copy link
Member Author

Ekleog commented Mar 4, 2018

ln -s indeed, thanks!

Mic92
Mic92 reviewed Mar 4, 2018
View reviewed changes
pkgs/applications/networking/instant-messengers/riot/riot-web.nix Outdated
@@ -1,5 +1,6 @@
{ stdenv, fetchurl, fetchpatch }:
{ lib, stdenv, fetchurl, fetchpatch, writeText, config ? null }:
Copy link
Member

Choose a reason for hiding this comment

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

Mhm, conf is a pretty generic term that might be set accidentially in all-packages.nix

Adding

riot-web = callPackage ../applications/networking/instant-messengers/riot/riot-web.nix {
  conf = config.riot-web.conf or null;
};

to all-packages.nix helps to avoid namespace pollution.
It can be still overridden with the override function, but it can be also set in:
~/.config/nixpkgs/config.nix via riot-web.conf
I think renaming it from conf to config also don't hurt.

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 guess you meant renaming from config to conf? Anyway you're right, I didn't think about the possibility of name conflict, changes applied, thank you :)

@Mic92 Mic92 merged commit 4ee9d05 into NixOS:master Mar 4, 2018
@Ekleog
Copy link
Member Author

Ekleog commented Mar 4, 2018

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Ekleog @Mic92 @GrahamcOfBorg