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

mangohud: add bitness suffix to layer name #231874

Merged
merged 2 commits into from
May 15, 2023

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented May 14, 2023

Description of changes

The VK loader finds the 32-bit layer first and does not attempt to load the
64-bit layer afterwards; likely because it shares the same name. Simply giving
them different names fixes the issue; both layers are tried and the correct one
succeeds.

A similar patter is employed by obs-vkcapture which continued working after
#228870.

Fixes #230978

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.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.

The VK loader finds the 32-bit layer first and does not attempt to load the
64-bit layer afterwards; likely because it shares the same name. Simply giving
them different names fixes the issue; both layers are tried and the correct one
succeeds.

A similar patter is employed by obs-vkcapture which continued working after
NixOS#228870.

Fixes NixOS#230978
Comment on lines +208 to +209
substituteInPlace $out/share/vulkan/implicit_layer.d/MangoHud.${layerPlatform}.json \
--replace "VK_LAYER_MANGOHUD_overlay" "VK_LAYER_MANGOHUD_overlay_${toString stdenv.hostPlatform.parsed.cpu.bits}"
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 don't have the means to test but if MangoApp needs this too, the file should be modified here aswell when support is enabled.

cc @Scrumplex

Copy link
Contributor

@kira-bruneau kira-bruneau May 14, 2023

Choose a reason for hiding this comment

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

Yep, this should be done for MangoApp too!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait sorry nvm!

Copy link
Contributor

@kira-bruneau kira-bruneau May 14, 2023

Choose a reason for hiding this comment

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

Right now our current build isn't even supporting mixed 32bit & 64bit mangoapp (and I don't think upstream does either). Since it runs in a separate process, there's no need to have a 32bit build for overlaying 32bit games (on a 64bit system).

@K900
Copy link
Contributor

K900 commented May 14, 2023

Maybe we can get this upstreamed? Shouldn't be hard to add the arch suffix directly to the template.

This allows the user to disable 32-bit support for closure size reasons or in
order to mitigate loader issues like
NixOS#230978.

The name is weird because it can't start with a digit :/
@Atemu Atemu force-pushed the fix/mangohud-layer-name-bitness branch from c3483ff to 7eacc7f Compare May 14, 2023 16:36
@Atemu
Copy link
Member Author

Atemu commented May 14, 2023

I'll create an upstream issue.

cc @THERAAB @IceDBorn for testing.

Copy link
Contributor

@kira-bruneau kira-bruneau left a comment

Choose a reason for hiding this comment

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

Still want to do a bit more testing (having some other problems with my system right now that's blocking me), but the changes look good to me!

Thanks for taking this on @Atemu & @K900 ❤️

@THERAAB
Copy link

THERAAB commented May 14, 2023

Hi @Atemu @kira-bruneau I'm happy to test this, but unsure of how I can do so? Should I replace nixpkgs reference in my flake.nix with this branch Atemu:fix/mangohud-layer-name-bitness?

@IceDBorn
Copy link
Contributor

It works like a charm! Thank you @Atemu

@Atemu
Copy link
Member Author

Atemu commented May 15, 2023

@THERAAB I don't use flakes but I'd expect that to work. Overriding the NIX_PATH would be the non-flakes way of doing it. My branch is based on last week's unstable.

@Atemu Atemu merged commit 746a5fc into NixOS:master May 15, 2023
@Atemu Atemu deleted the fix/mangohud-layer-name-bitness branch May 15, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: clean-up 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mangohud no longer appears when loading games
5 participants