umu-launcher[-unwrapped]: init at 1.1.4 #369259

diniamo · 2024-12-29T23:33:38Z

This was a tough one, but my version still turned out miles better than what they have in their repo (do not look at it if you care about your eyesight).

Things done

Add a 👍 reaction to pull requests you find important.

diniamo · 2024-12-29T23:35:39Z

Oh yeah, the build fails because of #366359, I've been working on this with doCheck = false in hatch's package definition.

pkgs/by-name/um/umu-launcher-unwrapped/package.nix

MattSturgeon · 2025-01-03T23:41:12Z

pkgs/by-name/um/umu-launcher/package.nix

+  # HACK: This has to be umu-run for the executable to be called that, but the
+  # package attribute is still umu-launcher. Is there a better way?
+  pname = "umu-run";


Looks like it requires changes to buildFHSEnv

I was looking for the implementation in https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/build-fhsenv-bubblewrap/buildFHSEnv.nix, but I believe it's actually this line in default.nix:

nixpkgs/pkgs/build-support/build-fhsenv-bubblewrap/default.nix

Line 52 in 268aaf5

executableName = args.pname or args.name;

Making executableName a de-structured function arg should solve this limitation.

Not in the scope of those PR. Changing basic builders probably also has a convoluted process so I don't think I will bother.

Agreed it's not really in scope for this PR. Was just raising the discussion as it was something you'd highlighted with your comments.

executableName is now merged

pkgs/by-name/um/umu-launcher-unwrapped/makefile-installer-prefix.patch

pkgs/by-name/um/umu-launcher-unwrapped/no-umu-version-json.patch

beh-10257 · 2025-01-07T19:27:09Z

do not look at it if you care about your eyesight

instead of saying this type of stuff why don't you tell me what are your complaints and actually make an improvement

diniamo · 2025-01-07T19:40:22Z

Sorry. The improvement is this package, since I saw more value in upstreaming a proper package to nixpkgs compared to fixing the problems in the flake.

diniamo · 2025-01-07T19:43:35Z

On another note, @MattSturgeon please mark the threads as resolved if they are.

Also, should I add you and @beh-10257 as maintainers to the package (assuming you're already in the maintainers list)?

beh-10257 · 2025-01-07T19:45:45Z

the flake is always gonna be more up to date so there is value in that
yes add me to the maintainers list
can you list the improvements so I can make the flake better like I'm trying to make things better here so cooporate

MattSturgeon · 2025-01-07T19:50:45Z

On another note, @MattSturgeon please mark the threads as resolved if they are.

I don't have permission to do that. I think as PR author you can? Otherwise someone with write-access must do it.

diniamo · 2025-01-07T19:51:43Z

Huh? You should be able to resolve your own reviews. Well in that case, do you want to add anything else or can I resolve them?

diniamo · 2025-01-07T19:59:29Z

...can you list the improvements so I can make the flake better...

@beh-10257 most of them you can get by replacing the package definitions entirely with the ones from this PR, while adapting them for the flake (make sure to tell me about anything that's missing from the packages in this PR along with a good reasoning as well, so I can add them too; eg. optional dependencies, since I couldn't find anything about those, but I did see some in the Nix package). Once you do that, I'll PR the rest of my suggestions, since that's easier than explaining.

MattSturgeon · 2025-01-07T20:11:20Z

Huh? You should be able to resolve your own reviews.

From Resolving conversations:

You can resolve a conversation in a pull request if you opened the pull request or if you have write access to the repository where the pull request was opened.

Well in that case, do you want to add anything else or can I resolve them?

Most of my threads were more discussion than feedback. If you feel that they are resolved, then feel free to mark as such.

the flake is always gonna be more up to date so there is value in that
can you list the improvements so I can make the flake better like I'm trying to make things better here so cooporate

I agree it'd be nice if the flake and nixpkgs were kept in sync. This will likely need breaking changes though.

One way that could be done would be to have the flake base itself on the nixpkgs impl, simply overriding src to point to the upstream repo. E.g:

# flake outputs
{
  overlays.default = final: prev: {
    umu-launcher-unwrapped = prev.umu-launcher-unwrapped.overrideAttrs (old: {
      # TODO: would be nice to filter out irrelevant dirs,
      # to avoid them invalidating build caches
      # e.g. using filesets
      src = ../../.;

      # TODO: version should also be something sensible
      version = "?";
    });
  };
}

One way to avoid that being an invisibly breaking change, would be to add a new flake.nix to the root of the upstream repo.

By keeping the old flake at its existing location (with a warning), users would have time to migrate to the new flake at the new location.

That way a) end-users don't need ?dir=packaging/nix and b) the old flake can be left as-is for a period of migration.

diniamo · 2025-01-07T20:28:33Z

Not needing ?dir=packaging/nix would be the breaking change, as it's been there previously. Also yeah, an override-based approach would be nice.

MattSturgeon · 2025-01-07T20:38:08Z

Also, should I add you and @ beh-10257 as maintainers

Sorry I missed that question. Feel free to add me 👍

LovingMelody · 2025-01-11T17:08:49Z

Please add me to the maintainers list as well (fuzen)

diniamo · 2025-01-11T18:03:24Z

Added the 2 of you, but I couldn't find @beh-10257

LovingMelody · 2025-01-17T01:16:52Z

pkgs/by-name/um/umu-launcher/package.nix

@@ -0,0 +1,15 @@
+{ buildFHSEnv, umu-launcher-unwrapped }:


May be worth copying heroic

nixpkgs/pkgs/games/heroic/fhsenv.nix

Lines 4 to 5 in 235caa7

extraPkgs ? pkgs: [ ],

extraLibraries ? pkgs: [ ],

To make it easier for end-users to add extra packages to the FHS environment by overriding.

This could be done in a follow-up PR if preferred

Yes I what the code does, but in what scenario would that be useful

I assume heroic launcher also supports native games, in which case it would make sense, but as far as I know, umu is for windows games only

Needed for passing graphics and fixing a cursor issue. Graphics is needed for dxvk-nvapi to work

nixpkgs/nixos/modules/programs/steam.nix

Lines 51 to 57 in e24b4c0

extraLibraries = pkgs: let

prevLibs = if prev ? extraLibraries then prev.extraLibraries pkgs else [ ];

additionalLibs = with config.hardware.graphics;

if pkgs.stdenv.hostPlatform.is64bit

then [ package ] ++ extraPackages

else [ package32 ] ++ extraPackages32;

in prevLibs ++ additionalLibs;

diniamo · 2025-01-17T05:08:42Z

Fixed formatting

MattSturgeon

Oh yeah, the build fails because of #366359, I've been working on this with doCheck = false in hatch's package definition.

The fix (#371743) is currently in staging-next. Not sure how long until that'll be merged into master.

MattSturgeon · 2025-01-17T11:13:32Z

pkgs/by-name/um/umu-launcher-unwrapped/no-umu-version-json.patch

+-	sed 's|##UMU_VERSION##|$(shell git describe --always --long --tags)|g' -i $(<).tmp
+	sed 's|##UMU_VERSION##|@version@|g' -i $(<).tmp


This patch shouldn't be needed once Open-Wine-Components/umu-launcher#289 makes it into a release.

Maybe gate the patch file behind lib.versionOlder "1.1.5" or `version == "1.1.4"? Assuming this PR gets merged before there's another upstream release.

That way anyone overriding this package with a newer version (or commit) won't also need to disable the patch.

MattSturgeon · 2025-01-17T11:14:13Z

pkgs/by-name/um/umu-launcher-unwrapped/no-umu-version-json.patch

+-RELEASEDIR := $(PROJECT)-$(shell git describe --abbrev=0)
+RELEASEDIR := $(PROJECT)-@version@


This patch could be avoided by overriding RELEASEDIR explicitly in makeFlags.

If it matters, we know $(PROJECT) should always be umu-launcher. But I think for our purposes RELEASEDIR could be any string?

MattSturgeon · 2025-01-17T11:16:33Z

pkgs/by-name/um/umu-launcher-unwrapped/package.nix

+  };
+
+  patches = [
+    ./makefile-installer-prefix.patch


Could you add a comment to explain why this patch is needed? And/or a TODO/FIXME if you think there's a better approach?

(personally, I don't really understand why we need --prefix instead of --destdir here)

MattSturgeon · 2025-01-17T11:26:07Z

pkgs/by-name/um/umu-launcher-unwrapped/package.nix

+  pyproject = false;
+  configureScript = "./configure.sh";
+
+  # The Makefile variables are a big mess, but this combination (and the patches) more or less do what we want


Nit: could we avoid vague negativity in comments? It could be helpful to list issues we have with the upstream variables, but I think this comment as-is may be counter-productive.

MattSturgeon · 2025-01-17T11:29:44Z

pkgs/by-name/um/umu-launcher-unwrapped/package.nix

+  preBuild = ''
+    makeFlagsArray+=(
+      "PYTHON_INTERPRETER=${lib.getExe python3Packages.python}"
+      "SHELL_INTERPRETER=${lib.getExe bash}"
+      "PREFIX="
+      "PYTHONDIR=/${python3Packages.python.sitePackages}"
+      "DESTDIR=$out"
+    )
+  '';


I know we touched on this before, but my preference would be to set this via makeFlags instead of having a preBuild script.

I think this would make the package easier to override. Probably not a big deal, but it feels cleaner.

This could be done by either

setting DESTDIR to some local path, then later during the installPhase moving the files to $out,

or by setting DESTDIR=${placeholder "out"}.

MattSturgeon · 2025-01-17T11:30:30Z

pkgs/by-name/um/umu-launcher/package.nix

+  targetPkgs = pkgs: [ pkgs.umu-launcher-unwrapped ];
+
+  executableName = "umu-run";
+  runScript = "${umu-launcher-unwrapped}/bin/umu-run";


Suggested change

runScript = "${umu-launcher-unwrapped}/bin/umu-run";

runScript = lib.getExe umu-launcher-unwrapped;

MattSturgeon · 2025-01-17T11:33:29Z

pkgs/by-name/um/umu-launcher/package.nix

+
+  targetPkgs = pkgs: [ pkgs.umu-launcher-unwrapped ];
+
+  executableName = "umu-run";


IDK if this is good practice, but this could be set as:

Suggested change

executableName = "umu-run";

executableName = umu-launcher-unwrapped.meta.mainProgram;

MattSturgeon · 2025-01-17T12:09:22Z

Also, should I add you and @beh-10257 as maintainers to the package (assuming you're already in the maintainers list)?

yes add me to the maintainers list

Added the 2 of you, but I couldn't find @beh-10257

It seems they aren't in the list. It'd be great to have them, but that'll have to do that themselves in a follow up PR.

@beh-10257 see the relevant documentation:

How to become a maintainer
comments at the top of maintainer-list.nix
Commit conventions

diniamo force-pushed the umu-launcher branch from ccb8609 to 6972537 Compare December 29, 2024 23:36

github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 29, 2024

diniamo force-pushed the umu-launcher branch from 6972537 to 42fdf85 Compare December 30, 2024 01:00

diniamo changed the title ~~umu-launcher: init at 1.1.4~~ umu-launcher[-unwrapped]: init at 1.1.4 Dec 30, 2024

diniamo force-pushed the umu-launcher branch 2 times, most recently from 73f963a to 9257d01 Compare December 30, 2024 01:41

ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes labels Dec 30, 2024

MattSturgeon reviewed Jan 4, 2025

View reviewed changes

diniamo force-pushed the umu-launcher branch from 9257d01 to 33d5d0a Compare January 4, 2025 07:02

diniamo force-pushed the umu-launcher branch from 33d5d0a to 3fb100f Compare January 11, 2025 18:03

diniamo force-pushed the umu-launcher branch from 3fb100f to f35f6fd Compare January 15, 2025 04:23

LovingMelody reviewed Jan 17, 2025

View reviewed changes

umu-launcher[-unwrapped]: init at 1.1.4

72fca19

diniamo force-pushed the umu-launcher branch from f35f6fd to 72fca19 Compare January 17, 2025 05:08

MattSturgeon reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

umu-launcher[-unwrapped]: init at 1.1.4 #369259

umu-launcher[-unwrapped]: init at 1.1.4 #369259

diniamo commented Dec 29, 2024

diniamo commented Dec 29, 2024 •

edited

Loading

MattSturgeon Jan 3, 2025 •

edited

Loading

diniamo Jan 4, 2025

MattSturgeon Jan 6, 2025

MattSturgeon Jan 7, 2025

MattSturgeon Jan 14, 2025

beh-10257 commented Jan 7, 2025

diniamo commented Jan 7, 2025

diniamo commented Jan 7, 2025

beh-10257 commented Jan 7, 2025

MattSturgeon commented Jan 7, 2025

diniamo commented Jan 7, 2025

diniamo commented Jan 7, 2025

MattSturgeon commented Jan 7, 2025 •

edited

Loading

diniamo commented Jan 7, 2025

MattSturgeon commented Jan 7, 2025 •

edited

Loading

LovingMelody commented Jan 11, 2025

diniamo commented Jan 11, 2025

LovingMelody Jan 17, 2025

diniamo Jan 17, 2025

MattSturgeon Jan 17, 2025

diniamo Jan 17, 2025

LovingMelody Jan 17, 2025

diniamo commented Jan 17, 2025

MattSturgeon left a comment

MattSturgeon Jan 17, 2025

MattSturgeon Jan 17, 2025

MattSturgeon Jan 17, 2025

MattSturgeon Jan 17, 2025

MattSturgeon Jan 17, 2025

MattSturgeon Jan 17, 2025

MattSturgeon Jan 17, 2025

MattSturgeon commented Jan 17, 2025

	extraLibraries = pkgs: let
	prevLibs = if prev ? extraLibraries then prev.extraLibraries pkgs else [ ];
	additionalLibs = with config.hardware.graphics;
	if pkgs.stdenv.hostPlatform.is64bit
	then [ package ] ++ extraPackages
	else [ package32 ] ++ extraPackages32;
	in prevLibs ++ additionalLibs;

		- sed 's\|##UMU_VERSION##\|$(shell git describe --always --long --tags)\|g' -i $(<).tmp
		+ sed 's\|##UMU_VERSION##\|@version@\|g' -i $(<).tmp

		-RELEASEDIR := $(PROJECT)-$(shell git describe --abbrev=0)
		+RELEASEDIR := $(PROJECT)-@version@

	runScript = "${umu-launcher-unwrapped}/bin/umu-run";
	runScript = lib.getExe umu-launcher-unwrapped;


		targetPkgs = pkgs: [ pkgs.umu-launcher-unwrapped ];

		executableName = "umu-run";

	executableName = "umu-run";
	executableName = umu-launcher-unwrapped.meta.mainProgram;

umu-launcher[-unwrapped]: init at 1.1.4 #369259

Are you sure you want to change the base?

umu-launcher[-unwrapped]: init at 1.1.4 #369259

Conversation

diniamo commented Dec 29, 2024

Things done

diniamo commented Dec 29, 2024 • edited Loading

MattSturgeon Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beh-10257 commented Jan 7, 2025

diniamo commented Jan 7, 2025

diniamo commented Jan 7, 2025

beh-10257 commented Jan 7, 2025

MattSturgeon commented Jan 7, 2025

diniamo commented Jan 7, 2025

diniamo commented Jan 7, 2025

MattSturgeon commented Jan 7, 2025 • edited Loading

diniamo commented Jan 7, 2025

MattSturgeon commented Jan 7, 2025 • edited Loading

LovingMelody commented Jan 11, 2025

diniamo commented Jan 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diniamo commented Jan 17, 2025

MattSturgeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattSturgeon commented Jan 17, 2025

diniamo commented Dec 29, 2024 •

edited

Loading

MattSturgeon Jan 3, 2025 •

edited

Loading

MattSturgeon commented Jan 7, 2025 •

edited

Loading

MattSturgeon commented Jan 7, 2025 •

edited

Loading