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

thelounge: Set THELOUNGE_HOME environment variable #70318

Merged
merged 1 commit into from
May 1, 2020

Conversation

nuxeh
Copy link
Contributor

@nuxeh nuxeh commented Oct 3, 2019

Setting this environment variable allows the CLI to correctly locate the
state directory, allowing users to be added, etc. from the command line,
without additionally having to set this variable.

Motivation for this change
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 @Mrmaxmeier

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Oct 3, 2019
@flokli
Copy link
Contributor

flokli commented Oct 11, 2019

Hmm, I wouldn't suggest to set this environment variable globally.

Instead, we might want to patch the CLI to use the used state directory location as a default. Reading its function definition, this could be accomplished by adding a

echo /var/lib/thelounge > $out/lib/node_modules/thelounge/.thelounge_home

to the thelounge postInstall in pkgs/development/node-packages/default-v10.nix.

@Ekleog
Copy link
Member

Ekleog commented Apr 27, 2020

@nuxeh Are you still interested in moving this forward, with @flokli's comments? :)

@nuxeh
Copy link
Contributor Author

nuxeh commented Apr 28, 2020

At the time I was somewhat doubtful of my own aptitude for implementing or testing it, I personally haven't written a line of Javascript in a long time.

Also it's my instinct that this solution is slightly more error-prone (especially going forwards, it may need to be maintained) than simply setting an environment variable, which doesn't seem to be too much of an issue? It'll only be present when the lounge is installed after all, and is fairly well namespaced as THELOUNGE_HOME. I don't really know the conventions of Nix for this sort of thing, however.

It is something that would be nice to have a fix for, certainly. I could look at it at some point.

@nuxeh
Copy link
Contributor Author

nuxeh commented Apr 28, 2020

Haha, oh wait, I wasn't paying enough attention... the suggestion isn't to patch the Javascript at all.

@nuxeh
Copy link
Contributor Author

nuxeh commented May 1, 2020

@Ekleog just done this, seems to work nicely!

@ofborg ofborg bot added 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels May 1, 2020
@Ekleog
Copy link
Member

Ekleog commented May 1, 2020

Great! That said, I've just looked at the current state of this PR, and it looks to me like you could revert the changes in nixos/modules/services/networking/thelounge.nix now that the correct path is being set right into the CLI tools :)

@nuxeh
Copy link
Contributor Author

nuxeh commented May 1, 2020

Oh, right, yes, I totally forgot!

The output file is found and handled by thelounge itself [1], leaving
the user free to override THELOUNGE_HOME in the environment if they
choose, but having a sensible default to make `thelounge` generally
usable in most cases.

This solution follows discussion on NixOS#70318.

[1] https://github.com/thelounge/thelounge/blob/9ef5c6c67e463c1f401e33b21dfb5641636e5ed1/src/command-line/utils.js#L56
@flokli flokli merged commit 5f9a48d into NixOS:master May 1, 2020
@nuxeh nuxeh deleted the nuxeh/theloungeenv branch May 1, 2020 16:45
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants