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

@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 added a commit to nuxeh/nixpkgs that referenced this pull request May 1, 2020
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
@nuxeh nuxeh force-pushed the nuxeh:nuxeh/theloungeenv branch from 8832239 to edeb345 May 1, 2020
@nuxeh
Copy link
Contributor Author

nuxeh commented May 1, 2020

@Ekleog just done this, seems to work nicely!

@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 #70318.

[1] https://github.com/thelounge/thelounge/blob/9ef5c6c67e463c1f401e33b21dfb5641636e5ed1/src/command-line/utils.js#L56
@nuxeh nuxeh force-pushed the nuxeh:nuxeh/theloungeenv branch from edeb345 to df2f8d9 May 1, 2020
@flokli flokli merged commit 5f9a48d into NixOS:master May 1, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df2f8d9"; rev="df2f8d915051d3d494ba7cb572c66c84bef84dcf"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df2f8d9"; rev="df2f8d915051d3d494ba7cb572c66c84bef84dcf"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df2f8d9"; rev="df2f8d915051d3d494ba7cb572c66c84bef84dcf"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df2f8d9"; rev="df2f8d915051d3d494ba7cb572c66c84bef84dcf"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df2f8d9"; rev="df2f8d915051d3d494ba7cb572c66c84bef84dcf"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df2f8d9"; rev="df2f8d915051d3d494ba7cb572c66c84bef84dcf"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="df2f8d9"; rev="df2f8d915051d3d494ba7cb572c66c84bef84dcf"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
thelounge, thelounge.passthru.tests on aarch64-linux Success
Details
thelounge, thelounge.passthru.tests on x86_64-linux Success
Details
@nuxeh nuxeh deleted the nuxeh:nuxeh/theloungeenv branch May 1, 2020
@Mrmaxmeier Mrmaxmeier mentioned this pull request Jun 10, 2020
1 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.