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

Jump to bottom

thelounge: Set THELOUNGE_HOME environment variable #70318

Merged
merged 1 commit into from May 1, 2020
Merged

thelounge: Set THELOUNGE_HOME environment variable #70318

merged 1 commit into from May 1, 2020

Conversation

nuxeh
Copy link
Contributor

@nuxeh nuxeh commented Oct 3, 2019 •
edited

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

@nuxeh
thelounge: write out default path for thelounge
df2f8d9 
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
@Mrmaxmeier Mrmaxmeier mentioned this pull request Jun 10, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nuxeh @flokli @Ekleog @Janik-Haag