build(nix): minor fixes #384
Conversation
- Use configure flags to disable building vendored dependencies - Ensure pystd is always installed - it isn't optional - Move cargo lock to before dependencies
MattSturgeon
force-pushed
the
nix/small-fixes
branch
from
9f221bd to
41a2005
Compare
| preCheck = '' | ||
| ${prev.preCheck or ""} | ||
|
|
||
| # Some tests require a writable HOME |
There was a problem hiding this comment.
I'll have to rethink and improve the tests later because we shouldn't have to write to $HOME. It may require making the relevant functions more testable by accepting $XDG_DATA_HOME as a new parameter instead of calling Path.home(), which should be easy to do.
There was a problem hiding this comment.
In addition to not writing to $HOME, the unit tests needs to be refactored to creating temporary directories in the system's tmpfs whenever possible.
Both of those changes will be large and done at a later time in a separate pull request. Until that's merged, I'm not really comfortable with suggesting to run the unit tests in nixpkgs.
There was a problem hiding this comment.
I agree, unit tests writing to (or reading from) HOME is not ideal. It's inherently impure and will result in tests that are not pure or idempotent.
However since nix runs the tests in a sandbox and we have to use a temp HOME if we want it to be writable anyway, this isn't a blocker from nixpkgs perspective.
There was a problem hiding this comment.
writableTmpDirAsHomeHook was merged into nixpkgs a couple weeks ago, so I'll use that in the downstream package. Could use it here too, but it requires bumping the lockfile.
There was a problem hiding this comment.
Sure. Let’s use writableTmpDirAsHomeHook then merge this PR afterward
| ]; | ||
|
|
||
| disabledTests = [ | ||
| # Broken? Asserts that $STEAM_RUNTIME_LIBRARY_PATH is non-empty |
There was a problem hiding this comment.
If it's empty then it implies ldconfig does not exist or it does, but could not find the shared library paths of the system. This varies by distribution, and in Arch Linux it's /usr/lib and /usr/lib/libfakeroot at the very least. In addition to those, umu-launcher includes the game directory, but the expectation is for there to be at least 2 paths.
There was a problem hiding this comment.
For NixOS I can imagine it can vary across systems. For home manager, which I use, the path is:
STEAM_RUNTIME_LIBRARY_PATH=/usr/lib64:/usr/lib32:/nix/store/m2mwcap0d7ylsap005vpr6xan063jlax-glibc-2.40-66/lib
This was run with the command PROTONPATH=GE-Proton GAMEID=umu-0 umu-run ""
There was a problem hiding this comment.
I think the test is OK to be skipped here, and I'm sorta thinking an e2e test may be more appropriate instead of a unit test here...
There was a problem hiding this comment.
I suspect the issue here is that we run the tests within the same sandbox environment that's used to build the package.
Therefore impure environment variables, including things related to Steam, are not set.
This is an intentional design decision intended to keep nix packages "pure" and deterministic.
Is the test assuming it is running on a fully installed system?
| # Broken? Asserts that $STEAM_RUNTIME_LIBRARY_PATH is non-empty | ||
| # Fails with AssertionError: '' is not true : Expected two elements in STEAM_RUNTIME_LIBRARY_PATHS | ||
| "test_game_drive_empty" | ||
| "test_game_drive_libpath_empty" |
There was a problem hiding this comment.
These tests failing are related to the above.
https://github.com/Open-Wine-Components/umu-launcher/releases/tag/1.2.0 https://github.com/Open-Wine-Components/umu-launcher/releases/tag/1.2.1 https://github.com/Open-Wine-Components/umu-launcher/releases/tag/1.2.2 https://github.com/Open-Wine-Components/umu-launcher/releases/tag/1.2.3 https://github.com/Open-Wine-Components/umu-launcher/releases/tag/1.2.4 https://github.com/Open-Wine-Components/umu-launcher/releases/tag/1.2.5 Incorporates work done in: Open-Wine-Components/umu-launcher#345 Open-Wine-Components/umu-launcher#384
Based on @R1kaB3rN's feedback in NixOS/nixpkgs#381975
Pushed up separately to #374, so that we can keep that one small & atomic.
propagatedBuildInputstopythonPathcc @beh-10257 @LovingMelody @R1kaB3rN