retroArchCores: remove, retroarchFull: init, retroarch: use fixed paths on "libretro_info_path" by thiagokokada · Pull Request #146714 · NixOS/nixpkgs

thiagokokada · 2021-11-20T00:29:41Z

Motivation for this change

retroArchCores is strange: it requires a global configuration on nixpkgs, as:

nixpkgs.config.retroarch = {
  enableDolphin = true;
  enableMGBA = true;
  enableMAME = true;
};

To do so, we ended up declaring all available emulators on all-packages.nix. Failing to do so would mean that the emulator wouldn't be available.

However, there is a mechanism on nixpkgs that also works: overrides. Overrides are similar on how other packages works, for example:

(retroarch.override { cores = with libretro; [ citra snes9x ]; });

So let's remove retroArchCores and leave the overrides mechanism instead.

Things done

thiagokokada · 2021-11-20T00:30:15Z

Maybe this needs an entry on release notes 🤔 ?

thiagokokada · 2021-11-20T00:31:18Z

CC @aanderse.

BTW, this doesn't invalidate the need of programs.retroarch module. It is basically just a clean-up because I think retroArchCores is strange as a mechanism to add cores to packages.

thiagokokada · 2021-11-20T00:32:20Z

pkgs/misc/emulators/retroarch/kodi-advanced-launchers.nix

Needed to remove the assertion since it would evaluate the assertion before the override occurs.

But I tested and this works fine too:

(kodi-retroarch-advanced-launchers.override { cores = with pkgs.libretro; [ snes9x desmume ]; })

pkgs/top-level/all-packages.nix

aanderse · 2021-11-20T00:52:27Z

I'm not sure about this. If I run retroarch for the first time then retroarch.cfg will be created in my XDG_CONFIG_HOME directory with the correct values for libretro_directory and libretro_info_path (because the values will be copied in from ${pkgs.retroarch}/etc/retroarch.cfg) - but as soon as I do a nixpkgs update + garbage collection then both libretro_directory and libretro_info_path will be pointing to a path that doesn't exist. I think we need a module now.

thiagokokada · 2021-11-20T01:07:07Z

I'm not sure about this. If I run retroarch for the first time then retroarch.cfg will be created in my XDG_CONFIG_HOME directory with the correct values for libretro_directory and libretro_info_path (because the values will be copied in from ${pkgs.retroarch}/etc/retroarch.cfg) - but as soon as I do a nixpkgs update + garbage collection then both libretro_directory and libretro_info_path will be pointing to a path that doesn't exist. I think we need a module now.

No because when the path doesn't exist RetroArch will update the config with defaults (that is now up-to-date).

Of course, not ideal, but works 🤷‍♂️ .

BTW, this is not related to this PR right 🤔 ?

aanderse · 2021-11-20T01:09:38Z

No because when the path doesn't exist RetroArch will default to the default config (that is now up-to-date).

Ah, sorry. I missed that part.

So... should we be consistent and do retroarch.withPackages (p: with p; [ ... ]) instead of retroarch.override { cores = with pkgs.libretro; [ ... ] }? 🤔

aanderse · 2021-11-20T01:10:20Z

Maybe this needs an entry on release notes thinking ?

Yes!

thiagokokada · 2021-11-20T01:14:00Z

So... should we be consistent and do retroarch.withPackages (p: with p; [ ... ]) instead of retroarch.override { cores = with pkgs.libretro; [ ... ] }? thinking

I mean, we could, but I still prefer the module because the situation is not ideal (for example, if I added/remove a core, until I trigger the GC this will reflect the old cores from the old derivation).

However, this is orthogonal with this PR. Even with the programs.retroarch we will need to support non-NixOS systems, e.g.: macOS. In this case the only option they have is to use either override or withPackages, even if it is sub-optiomal.

Now, adding withPackages mechanism and still having the issue doesn't seem worth it for me IMO.

So my proposal is to use overrides AND add programs.retroarch. However I think the module can come in another PR.

thiagokokada · 2021-11-20T01:18:36Z

However, this is orthogonal with this PR. Even with the programs.retroarch we will need to support non-NixOS systems, e.g.: macOS. In this case the only option they have is to use either override or withPackages, even if it is sub-optiomal

BTW, I want to eventually solve this problem, however I am trying to ask upstream for help so we don't need to patch too much. See issue libretro/RetroArch#13251 for details.

Even if a better solution for the core issue came afterwards, I still think that having a module is worth it. With it, we can allow folks to fully customize RetroArch (for example, this would make NixOS a good replacement for RetroPie).

Does this make sense @aanderse ? If you want, feel free to ask me more details on Matrix.

thiagokokada · 2021-11-20T01:46:46Z

pkgs/top-level/all-packages.nix

I just discovered that this one didn't even work (the correct name is thepowedertoy).

thiagokokada · 2021-11-20T02:18:02Z

Yes!

Done.

Also added a release notes entry about the recent RetroArch bump.

thiagokokada · 2021-11-20T15:31:06Z

Result of nixpkgs-review pr 146714 run on x86_64-linux 1

1 package blacklisted:

tests.nixos-functions.nixos-test

1 package failed to build:

nixos-install-tools

2 packages built:

kodi-retroarch-advanced-launchers
retroarchFull

thiagokokada · 2021-11-20T16:08:30Z

@aanderse Can I have your review?

thiagokokada · 2021-11-20T19:14:54Z

BTW, I want to eventually solve this problem, however I am trying to ask upstream for help so we don't need to patch too much. See issue libretro/RetroArch#13251 for details.

With commit ~~f69afeb~~ c0ffe63, I finally solved this issue. Now RetroArch completely ignores the value of libretro_info_path (it already ignored libretro_directory) is set on retroarch.cfg. Better yet, it always updates the value on the configuration file, so this is less confusing to the users.

WDYT @aanderse ? I think this solves most of the issues. I can also split this in another PR if you want.

`retroArchCores` is strange: it requires a global configuration on nixpkgs, as: ```nix nixpkgs.config.retroarch = { enableDolphin = true; enableMGBA = true; enableMAME = true; }; ``` To do so, we ended up declaring all available emulators on `all-packages.nix`. Failing to do so would mean that the emulator wouldn't be available. However, there is a mechanism on nixpkgs that also works: overrides. Overrides are similar on how other packages works, for example: ```nix (retroarch.override { cores = with libretro; [ citra snes9x ]; }); ``` So let's remove `retroArchCores` and leave the overrides mechanism instead.

This is retroarch + all available retroarch cores.

aanderse

This looks great! It all makes sense to me, and the release notes cover everything we need to. Thanks for doing all of this 🙇

thiagokokada · 2021-11-20T19:31:10Z

This looks great! It all makes sense to me, and the release notes cover everything we need to. Thanks for doing all of this bow

Nice.

I will just test this on macOS since I need to patch one more place to make it work.

This commit introduces a patch that hardcodes "libretro_info_path" directly in the RetroArch code, without the issues of the previous approach. With this commit, RetroArch stops reading "libretro_info_path" from `retroarch.cfg` file, and always use the default.

With those changes retroarch builds on Darwin, but the executable itself is broken.

thiagokokada · 2021-11-20T19:44:18Z

@aanderse Sorry for bothering you, but can you do one last review with the Darwin changes?

aanderse · 2021-11-20T19:48:09Z

pkgs/misc/emulators/retroarch/default.nix

    maintainers = with maintainers; [ MP2E edwtjo matthewbauer kolbycrouch thiagokokada ];
+    # FIXME: exits with error on macOS:
+    # No Info.plist file in application bundle or no NSPrincipalClass in the Info.plist file, exiting
+    broken = stdenv.isDarwin;


I just saw a ton of people volunteer to support OSX recently. I wonder if we could find some volunteers after we merge this?

Yeah, this is my idea.

BTW, it is already broken on master, because Wayland is Linux-only (so it propagates its broken state).

On master:

$ nix-build -A retroarch error: Package ‘wayland-1.19.0’ in /Users/thiagoko/Projects/nixpkgs/pkgs/development/libraries/wayland/default.nix:107 is marked as broken, refusing to evaluate. a) To temporarily allow broken packages, you can use an environment variable for a single invocation of the nix tools. $ export NIXPKGS_ALLOW_BROKEN=1 b) For `nixos-rebuild` you can set { nixpkgs.config.allowBroken = true; } in configuration.nix to override this. c) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add { allowBroken = true; } to ~/.config/nixpkgs/config.nix. (use '--show-trace' to show detailed location information)

aanderse · 2021-11-20T19:49:14Z

No bother at all. Looks good to me 👍

thiagokokada · 2021-11-20T19:50:55Z

No bother at all. Looks good to me +1

Nice, I will do one last build to make sure everything is fine and merge them.

aanderse · 2021-11-20T19:52:52Z

Great. Thanks again! I'll try to create a maintainers team PR this weekend.

I'm really loving to libretro integration in kodi 😄

matthewbauer · 2021-11-20T21:57:46Z

pkgs/misc/emulators/retroarch/0001-Disable-menu_show_core_updater.patch

+
+ # If disabled, will hide the ability to update cores (and core info files) inside the menu.
+-# menu_show_core_updater = true
+menu_show_core_updater = false


Does this disable manually installed cores too? I don’t know if anyone actually does this but I thought in the past you could imperitively install cores in the UI. If this can be enabled manually in the UI that would be alright.

No, it just disable the UI to download new cores, that didn't work anyway since it doesn't connect to the RetroArch core servers (it would just show the cores that you already have installed). I don't know if this used to work, but at least the situation from version 1.8.5 (the version that was on master before I started updating and refactor everything) was already like this.

You can still re-enable this manually though, and this is respected on the next boot. But it will be useless.

BTW, even if this used to work I don't think download would do anything.

First because this is NixOS, and thanks to this a core downloaded from RetroArch servers would probably just segfault (each core is a .so file and are dynamically linked, however since NixOS has no FHS it wouldn't find the necessary libraries).

Second because even before this PR, libretro_directory was already set to a path on /nix/store, that is read-only, so it would fail to write the core there. I am just setting libretro_directory directly in the code for consistency, but it was already the default.

matthewbauer · 2021-11-20T22:05:36Z

pkgs/misc/emulators/retroarch/0002-Use-fixed-paths-on-libretro_info_path.patch

+-   fill_pathname_join(g_defaults.dirs[DEFAULT_DIR_CORE], base_path,
+-         "cores", sizeof(g_defaults.dirs[DEFAULT_DIR_CORE]));
+   fill_pathname_join(g_defaults.dirs[DEFAULT_DIR_CORE], "@libretro_directory@",
+         "", sizeof(g_defaults.dirs[DEFAULT_DIR_CORE]));


Could we set the DEFAULT_DIR_CORE constant instead? If I understand this, it is assuming the default is unset?

No, this is just a enum. This is exactly the part of the code that sets the g_defaults global (that stores the default values) to the default.

I tried to set it before but got either a compilation failure or segfault (don't remember exactly).

thiagokokada · 2021-11-20T22:15:23Z

@matthewbauer Any other issue? Can I go and merge this (still building everything, but the only core that remains are MAME and should finish soon).

MAME since 0.225 have a fix for the build issues while building in parallel. Since libretro.mame is on 0.227 right now, should be safe to enable. Since eventually enableParallelBuilding should be the default, enabling it for all cores except the older MAMEs seems better than just enabling for libretro.mame.

thiagokokada · 2021-11-20T23:36:47Z

Saw this PR that should be included from MAME 0.225 and above (we use 0.227 currently), so I think it is safe to enable parallel building in everything, except for older MAME cores: mamedev/mame#7279.

Restart my build from zero (hope this is faster though, building the latest version of MAME in special is very slow without parallel building).

thiagokokada · 2021-11-21T01:21:09Z

Saw this PR that should be included from MAME 0.225 and above (we use 0.227 currently), so I think it is safe to enable parallel building in everything, except for older MAME cores: mamedev/mame#7279.

Restart my build from zero (hope this is faster though, building the latest version of MAME in special is very slow without parallel building).

Seems like it was a good idea, it build much faster:

Result of nixpkgs-review run on x86_64-linux 1

1 package failed to build:

nixos-install-tools

82 packages built:

kodi-retroarch-advanced-launchers (xbmc-retroarch-advanced-launchers)
kodiPackages.libretro-genplus
kodiPackages.libretro-mgba
kodiPackages.libretro-snes9x
libretro.atari800
libretro.beetle-gba
libretro.beetle-lynx
libretro.beetle-ngp
libretro.beetle-pce-fast
libretro.beetle-pcfx
libretro.beetle-psx
libretro.beetle-psx-hw
libretro.beetle-saturn
libretro.beetle-saturn-hw
libretro.beetle-snes
libretro.beetle-supergrafx
libretro.beetle-vb
libretro.beetle-wswan
libretro.bluemsx
libretro.bsnes-mercury
libretro.citra
libretro.desmume
libretro.desmume2015
libretro.dolphin
libretro.dosbox
libretro.eightyone
libretro.fbalpha2012
libretro.fbneo
libretro.fceumm
libretro.flycast
libretro.fmsx
libretro.freeintv
libretro.gambatte
libretro.genesis-plus-gx
libretro.gpsp
libretro.gw
libretro.handy
libretro.hatari
libretro.mame
libretro.mame2000
libretro.mame2003
libretro.mame2003-plus
libretro.mame2010
libretro.mame2015
libretro.mame2016
libretro.mesen
libretro.meteor
libretro.mgba
libretro.mupen64plus
libretro.neocd
libretro.nestopia
libretro.np2kai
libretro.o2em
libretro.opera
libretro.parallel-n64
libretro.pcsx_rearmed
libretro.picodrive
libretro.play
libretro.ppsspp
libretro.prboom
libretro.prosystem
libretro.quicknes
libretro.sameboy
libretro.scummvm
libretro.smsplus-gx
libretro.snes9x
libretro.snes9x2002
libretro.snes9x2005
libretro.snes9x2010
libretro.stella
libretro.stella2014
libretro.tgbdual
libretro.thepowdertoy
libretro.tic80
libretro.vba-m
libretro.vba-next
libretro.vecx
libretro.virtualjaguar
libretro.yabause
retroarch
retroarchBare
retroarchFull

nixos-discourse · 2023-12-16T18:45:04Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/help-understanding-package-paths-in-nix-store/36682/13

thiagokokada commented Nov 20, 2021

View reviewed changes

thiagokokada changed the title ~~retroArchCore: remove~~ retroArchCores: remove Nov 20, 2021

thiagokokada commented Nov 20, 2021

View reviewed changes

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved

thiagokokada commented Nov 20, 2021

View reviewed changes

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Nov 20, 2021

thiagokokada changed the title ~~retroArchCores: remove~~ retroArchCores: remove, retroarchFull: init Nov 20, 2021

thiagokokada added 2 commits November 20, 2021 16:19

retroarchFull: init

cbcd3d6

This is retroarch + all available retroarch cores.

aanderse approved these changes Nov 20, 2021

View reviewed changes

thiagokokada added 3 commits November 20, 2021 16:34

nixos/doc: add release notes about retroarch changes

36f6fd1

retroarch: fix build on macOS, mark as broken

f435d7d

With those changes retroarch builds on Darwin, but the executable itself is broken.

thiagokokada changed the title ~~retroArchCores: remove, retroarchFull: init~~ retroArchCores: remove, retroarchFull: init, retroarch: use fixed paths on "libretro_info_path" Nov 20, 2021

aanderse reviewed Nov 20, 2021

View reviewed changes

matthewbauer reviewed Nov 20, 2021

View reviewed changes

ofborg bot requested review from hrdinka and matthewbauer November 20, 2021 23:47

thiagokokada merged commit ff4c097 into NixOS:master Nov 21, 2021

thiagokokada deleted the remove-retroArchCores branch November 21, 2021 01:22

This was referenced Nov 22, 2021

Add missing libretro cores #146958

Merged

create a libretro maintainers team? #146402

Closed

Uh oh!

Conversation

thiagokokada commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Uh oh!

thiagokokada commented Nov 20, 2021

Uh oh!

thiagokokada commented Nov 20, 2021

Uh oh!

thiagokokada Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aanderse commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thiagokokada commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aanderse commented Nov 20, 2021

Uh oh!

aanderse commented Nov 20, 2021

Uh oh!

thiagokokada commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thiagokokada commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thiagokokada Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

thiagokokada commented Nov 20, 2021

Uh oh!

thiagokokada commented Nov 20, 2021

Uh oh!

thiagokokada commented Nov 20, 2021

Uh oh!

thiagokokada commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aanderse left a comment

Choose a reason for hiding this comment

Uh oh!

thiagokokada commented Nov 20, 2021

Uh oh!

thiagokokada commented Nov 20, 2021

Uh oh!

aanderse Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

thiagokokada Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

thiagokokada Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aanderse commented Nov 20, 2021

Uh oh!

thiagokokada commented Nov 20, 2021

Uh oh!

aanderse commented Nov 20, 2021

Uh oh!

matthewbauer Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

thiagokokada Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

thiagokokada Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

matthewbauer Nov 20, 2021

Choose a reason for hiding this comment

Uh oh!

thiagokokada Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

thiagokokada commented Nov 20, 2021 •

edited

Loading

thiagokokada Nov 20, 2021 •

edited

Loading

aanderse commented Nov 20, 2021 •

edited

Loading

thiagokokada commented Nov 20, 2021 •

edited

Loading

thiagokokada commented Nov 20, 2021 •

edited

Loading

thiagokokada commented Nov 20, 2021 •

edited

Loading

thiagokokada commented Nov 20, 2021 •

edited

Loading

thiagokokada Nov 20, 2021 •

edited

Loading

thiagokokada Nov 20, 2021 •

edited

Loading