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

nixos/steam: add protontricks submodule #309585

Merged
merged 1 commit into from
May 24, 2024

Conversation

diniamo
Copy link
Contributor

@diniamo diniamo commented May 6, 2024

Description of changes

The title is self-explanatory, as for the reason: other programs also rely on this variable to find proton versions, for example protontricks.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@Shawn8901
Copy link
Contributor

FYI in the PR adding that it was explicitly asked to not expose that globally: see #189398 (comment)

@diniamo
Copy link
Contributor Author

diniamo commented May 6, 2024

I see. @Atemu what do you think now that it has become an issue? Creating a module for everything that relies on it would be tedious both for me and the user, I think a global variable is the way.

@Atemu
Copy link
Member

Atemu commented May 7, 2024

I don't think there's all that many tools which require steam-specific things. I'd be in favour of adding options for the one or two tools which actually need this. I honestly can't think of much else than protontricks.

As a general rule, one should never pollute the global namespace with env vars that only concern a handful of programs.

As an escape hatch, you can also always run the tool inside steam-run.

@diniamo
Copy link
Contributor Author

diniamo commented May 7, 2024

In that case, should I create a submodule like programs.steam.protontricks.{enable,package}?

@diniamo diniamo force-pushed the global-steam-compat-paths-var branch 2 times, most recently from 540eb19 to 2b8b941 Compare May 7, 2024 13:03
@diniamo
Copy link
Contributor Author

diniamo commented May 7, 2024

Tested, how is this?

Also, please point out formatting mistakes, not sure about this one.

@diniamo diniamo changed the title nixos/steam: expose STEAM_EXTRA_COMPAT_TOOLS_PATHS globally nixos/steam: add protontricks submodule May 7, 2024
nixos/modules/programs/steam.nix Outdated Show resolved Hide resolved
nixos/modules/programs/steam.nix Outdated Show resolved Hide resolved
nixos/modules/programs/steam.nix Outdated Show resolved Hide resolved
@diniamo diniamo force-pushed the global-steam-compat-paths-var branch 2 times, most recently from 1f7cc6c to febf2d7 Compare May 10, 2024 17:22
@diniamo
Copy link
Contributor Author

diniamo commented May 10, 2024

There we go, tested too, I'm still unsure about formatting however.

@Atemu
Copy link
Member

Atemu commented May 11, 2024

I'll take a closer look later but I don't see any obvious formatting issues. Unless you're doing something utterly weird that doesn't fit into the rest of Nixpkgs, there aren't any strict rules yet until NixOS/rfcs#166 is implemented.

Copy link
Contributor

@Shawn8901 Shawn8901 left a comment

Choose a reason for hiding this comment

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

lgtm

@diniamo
Copy link
Contributor Author

diniamo commented May 11, 2024

What is that ofborg error and where is the request change that's blocking the merge?

@Atemu
Copy link
Member

Atemu commented May 12, 2024

It's an internal error that someone who maintains ofBorg will have to take a look at and manually remove at some point. It doesn't block the merge though, that's just my review. It'll stop blocking it as soon as I approve after I've taken a proper look or another committer comes around and dismisses it.

@diniamo diniamo force-pushed the global-steam-compat-paths-var branch 2 times, most recently from bbf2920 to 50ab32c Compare May 15, 2024 16:39
@diniamo
Copy link
Contributor Author

diniamo commented May 15, 2024

Resolved conflicts, tested.

@diniamo
Copy link
Contributor Author

diniamo commented May 22, 2024

@Atemu have you had a chance to look? It would be great if we could get this in before the next unstable merge.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Two minor things, otherwise LGTM.

nixos/modules/programs/steam.nix Outdated Show resolved Hide resolved
@@ -139,6 +146,11 @@ in {
Load the extest library into Steam, to translate X11 input events to
uinput events (e.g. for using Steam Input on Wayland)
'';

protontricks = {
enable = lib.mkEnableOption "Protontricks";
Copy link
Member

Choose a reason for hiding this comment

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

Please consider users who may not know what protontricks is. The option to enable it should offer at least a little information on it.

We could also pull it from the package's meta like this:

Suggested change
enable = lib.mkEnableOption "Protontricks";
enable = lib.mkEnableOption "protontricks, ${cfg.protontricks.package.meta.description}";

@diniamo diniamo force-pushed the global-steam-compat-paths-var branch from 50ab32c to 95a1183 Compare May 23, 2024 18:23
@diniamo
Copy link
Contributor Author

diniamo commented May 23, 2024

Looks like your suggestion fails the documentation CI (and not just the CI, my system config errors too), apparently option descriptions can't rely on config/pkgs.

@diniamo diniamo force-pushed the global-steam-compat-paths-var branch from 95a1183 to fcaec6b Compare May 24, 2024 06:52
@diniamo
Copy link
Contributor Author

diniamo commented May 24, 2024

Welp, adding it manually is the best I can come up with.

@Atemu
Copy link
Member

Atemu commented May 24, 2024

Looks like your suggestion fails the documentation CI (and not just the CI, my system config errors too), apparently option descriptions can't rely on config/pkgs.

Ah, indeed it makes sense; we don't have access to config when defining options. You could probably dig it out of options..package.default but hard-coding like this is fine.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Diff LGTM and the NixOS part appears to be working but I'm not sure protontricks is as it's giving me:

protontricks (ERROR): Could not find configured Proton installation!
protontricks (ERROR): Active Proton installation could not be found automatically.
Proton installation could not be found!

Though I'm also getting the same error when I steam-run it which worked before?

Is it working on your end @diniamo?

@Atemu
Copy link
Member

Atemu commented May 24, 2024

Ah, wait I had to select a compat tool for the game once it seems; now it works flawlessly.

@Atemu Atemu merged commit 061a135 into NixOS:master May 24, 2024
21 checks passed
@Atemu
Copy link
Member

Atemu commented May 24, 2024

Thanks!

Atemu added a commit to Atemu/nixos-config that referenced this pull request May 24, 2024
@diniamo
Copy link
Contributor Author

diniamo commented May 24, 2024

Nice, thanks

diniamo pushed a commit to diniamo/nixpkgs that referenced this pull request May 28, 2024
…s-var

nixos/steam: add protontricks submodule
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants