-
-
Notifications
You must be signed in to change notification settings - Fork 16.5k
Heroic: fix helper programs #432183
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
base: master
Are you sure you want to change the base?
Heroic: fix helper programs #432183
Conversation
7a46b8c
to
614a93b
Compare
614a93b
to
981cd31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Did not test actual functionality. Not sure how to actually test these features without downloading GTA5. Added some suggestions.
nixpkgs-review
result for #432183
Generated using nixpkgs-review-gha
Command: nixpkgs-review pr 432183
Logs: https://github.com/keenanweaver/nixpkgs-review-gha/actions/runs/16851392980
x86_64-linux
✅ 4 packages built:
- galaxy-dummy-service
- heroic
- heroic-epic-integration
- heroic-unwrapped
aarch64-linux
✅ 2 packages built:
- galaxy-dummy-service
- heroic-epic-integration
x86_64-darwin
(sandbox = true)
✅ 2 packages built:
- galaxy-dummy-service
- heroic-epic-integration
aarch64-darwin
(sandbox = true)
✅ 2 packages built:
- galaxy-dummy-service
- heroic-epic-integration
981cd31
to
c64e5b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nixpkgs-review
result for #432183
Generated using nixpkgs-review-gha
Command: nixpkgs-review pr 432183
Logs: https://github.com/keenanweaver/nixpkgs-review-gha/actions/runs/16854007428
x86_64-linux
✅ 4 packages built:
- galaxy-dummy-service
- heroic
- heroic-epic-integration
- heroic-unwrapped
aarch64-linux
✅ 2 packages built:
- galaxy-dummy-service
- heroic-epic-integration
x86_64-darwin
(sandbox = true)
✅ 2 packages built:
- galaxy-dummy-service
- heroic-epic-integration
aarch64-darwin
(sandbox = true)
✅ 2 packages built:
- galaxy-dummy-service
- heroic-epic-integration
c64e5b5
to
1c064e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything functioning correctly as far as I can tell.
What's the maintenance you have in mind for all these different package versions? I can see things getting out of hand quickly with all the updates done to these components.
A bit out of my wheelhouse on this. Trying to understand if this is the best approach for the Heroic package. It just feels like there might be an easier way to do this. I looked around nixpkgs to see other examples of 'latest' vs. 'versioned' components. There are packages like podofo
and uhttpmock
which have distinct packages per version. In openrct2
, the dependencies are built into the same derivation (though those are just fetchurl
). Would it be desirable to do that for Heroic? Probably need someone more experienced than me to chime in on this.
nixpkgs-review
result for #432183
Generated using nixpkgs-review-gha
Command: nixpkgs-review pr 432183
Logs: https://github.com/keenanweaver/nixpkgs-review-gha/actions/runs/16907509795
x86_64-linux
✅ 6 packages built:
- comet-gog
- comet-gog_0_2_0
- galaxy-dummy-service_0_2_0
- heroic
- heroic-epic-integration_0_3
- heroic-unwrapped
aarch64-linux
✅ 4 packages built:
- comet-gog
- comet-gog_0_2_0
- galaxy-dummy-service_0_2_0
- heroic-epic-integration_0_3
x86_64-darwin
(sandbox = true)
✅ 4 packages built:
- comet-gog
- comet-gog_0_2_0
- galaxy-dummy-service_0_2_0
- heroic-epic-integration_0_3
aarch64-darwin
(sandbox = true)
✅ 4 packages built:
- comet-gog
- comet-gog_0_2_0
- galaxy-dummy-service_0_2_0
- heroic-epic-integration_0_3
This is likely not how you (intend to) do things in Nix land, but it'd be possible to just keep the binaries Heroic ships instead of building them yourself. FWIW this is how basically every other Linux package of Heroic works |
Ideally in Nix you want to build everything from source. For these Heroic components, though, I can see it being better to just put in the pre-compiled binaries, as I don't think these components are used in other software at this point in time. We can also have our cake and eat it too by having separate derivations for them (for the 'latest' versions only), and in the heroic package, just have the binaries downloaded. This is just my opinion and hope to see others' opinions. |
My plan is to manually bump them when Heroic releases come out, and see how that goes.
I think embedded derivations is generally discouraged. It would also make it difficult (or at least awkward) to override any of the package versions, and these packages can potentially be used outside of Heroic. |
}: | ||
|
||
rustPlatform.buildRustPackage (finalAttrs: { | ||
inherit (comet-gog) pname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should just set the pname
to "comet-gog"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep this inherit
, since we still want to reuse comet-gog.meta
further down the file.
0ec29e1
to
83cd05e
Compare
Here it is: #434055 |
For |
maintainers = with lib.maintainers; [ aidalgol ]; | ||
}; | ||
|
||
passthru.updateScript = gitUpdater { }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meta should always be written after passhru.
runHook preInstall | ||
|
||
mkdir $out | ||
cp GalaxyCommunication.exe $out/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be under $out/bin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure, since it's a Windows executable.
runHook preInstall | ||
|
||
mkdir $out | ||
cp heroic-epic-integration.exe $out/EpicGamesLauncher.exe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
We need to keep the helper programs at the same versions as upstream, so updating just this package is insufficient.
- comet-gog.dummy-service - epic-integration
83cd05e
to
83b1f0c
Compare
Cherry-picked all commits from #434055 and rebased this PR. |
This is a fork only for Heroic, so move the nix file for this derivation to beside heroic-unwrapped, but expose it as a passthru for ease of overriding.
83b1f0c
to
daf1d70
Compare
Getting an error in the logs, but the multiplayer component is still working. (As in, I can see servers and join)
When installing a new game. Again, multiplayer is seemingly working as intended.
GalaxyCommunication.exe isn't in either prefix's
|
Oops! Let me fix that. |
Turns out not to be a packaging problem. Heroic-Games-Launcher/HeroicGamesLauncher#4885 |
daf1d70
to
dae6e7c
Compare
@keenanweaver Could you try again with the latest changes? |
Yup, looks good now! It's in ProgramData and launching without error.
|
Fixes #431569
Closes #434055
Things done
passthru.tests
.nixpkgs-review
on this PR. See nixpkgs-review usage../result/bin/
.Add a 👍 reaction to pull requests you find important.