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

ryujinx-greemdev: init at 1.2.76 #353897

Merged
merged 1 commit into from
Dec 14, 2024
Merged

Conversation

Kek5chen
Copy link
Contributor

@Kek5chen Kek5chen commented Nov 5, 2024

Things done

The first thing I did is move to the QoL fork. The original repo is unsupported and will just go further and further out of date. There's no reason to keep it as the source.

The second thing I did was add support for aarch64-darwin, because the new M-Series MacBooks are actually really powerful.

I would guess that aarch64-linux and x86_64-darwin are now also supported, but I didn't test them. I would ask someone to please build them for me and tell me if they work. I will then adjust the PR. Thanks a lot in advance.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 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.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Nov 6, 2024
@FliegendeWurst FliegendeWurst added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 19, 2024
@Kek5chen
Copy link
Contributor Author

Kek5chen commented Nov 20, 2024

@Scrumplex I wanna notify you because you seem to have pushed 3b0e619 without making a PR. This is in straight conflict with my open PR. It would've been nice if you could've checked for open PRs before you did that, since this PR changes a lot about the package in general.

@Kek5chen Kek5chen force-pushed the ryujinx-darwin branch 2 times, most recently from 0a26e68 to 4fab3dd Compare November 20, 2024 13:48
@Kek5chen Kek5chen removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 20, 2024
@Kek5chen
Copy link
Contributor Author

@ofborg build ryujinx

@Kek5chen
Copy link
Contributor Author

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

1 package built:
  • ryujinx

@Scrumplex
Copy link
Member

@Scrumplex I wanna notify you because you seem to have pushed 3b0e619 without making a PR. This is in straight conflict with my open PR. It would've been nice if you could've checked for open PRs before you did that, since this PR changes a lot about the package in general.

That was actually part of this PR: #348601

We don't really have great tooling on GitHub to find potentially conflicting PRs easily, and I didn't really think the changes in my PR would cause big conflicts, but I am sorry if it did so for you. I'll try to avoid this in the future.

Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

As per commit conventions you should try to split up your commits to make it easier to review, as well as making it easier to backport individual changes.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Nov 21, 2024
@ofborg ofborg bot requested review from artemist and 06kellyjac November 21, 2024 09:28
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Nov 21, 2024
@06kellyjac
Copy link
Member

For future reference you should be able to see if a commit has a corresponding PR in the UI

image

It's not always possible to identify one via the cli because merge, rebase, squash strategies are all a bit different.
(If you can't see this it might be a feature of the Refined GitHub extension)

Copy link
Member

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

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

Firstly thank you for the PR and looking at aarch64-darwin compatibility. 🚀


The fork is relatively new.
If we are to adopt it now I think it should be named differently rather than treating it as a continuation. Other reasons to add it under a fresh name include the disclaimer in the README plus a logo change.

I'm in favor of keeping ryujinx og as is currently with the source mirror.

If you would like a version more preservative fork of Ryujinx, check out ryujinx-mirror.
(from the greemdev/ryujinx readme)

Adding ryujinx-mirror for the more og+ experience (as accepted by both projects)

And adding ryujinx-<something> for greemdev/ryujinx. I don't fully understand their distinction between not being a revival but also going beyond the og+ tweaks ryujinx-mirror are aiming for. I guess QoL means larger tweaks but still not significant changes that'd qualify for a revival project.

I'm thinking ryujinx-greemdev would be the most obvious but I'm open to hearing what the devs think.
I'd say ryujinx-qol is maybe a bit too much, I could imagine some other forks popping up offering ryujinx qol improvements. Maybe the project is interested in rebranding QoLjinx 😅 ?

Naming is hard as always but I'm just floating some ideas here. 🙂

TL;DR: I'd love to package the project but I think it needs a distinct name to differentiate it from og (same as I would want for the more true-to-og mirror project)

@Kek5chen
Copy link
Contributor Author

I was thinking ryujinx-qol at first too, and while I like that name more and think that there's not going to be other comparable projects popping up in the near future, I do think that it might be easier to find out more about the GreemDev fork by for example googling the ryujinx-greemdev package name or seeing it and making a quicker mental link to what it might be. So i'll just roll with that.

@Kek5chen Kek5chen changed the title ryujinx: add aarch64-darwin compatibility & update ryujinx-greemdev: init at 1.2.76 Nov 21, 2024
@Kek5chen Kek5chen added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Nov 21, 2024
@Kek5chen Kek5chen requested a review from 06kellyjac November 21, 2024 13:32
Copy link
Member

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

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

Looking good so far.

Remaining bits:

  1. fix formatting
  2. address the question comments
  3. change the update script to work for the greemdev/ryujinx repo and point to the right files & build target or remove it

Also before we merge this just double check people are happy maintaining this.

Since this is an additional package and not changing the og one (as I've covered in the comments earlier) I'm happy to also maintain this one

@Kek5chen
Copy link
Contributor Author

@06kellyjac

  1. Despite what the workflow says, formatting isn't broken. It's just the deps file that is auto generated in a readable format. Formatting it would make it really hard to read and diff.

  2. I don't know which question you're talking about. If you mean:

    Maybe the project is interested in rebranding QoLjinx 😅 ?
    Probably not? I don't really have the time to get something rolling over there too right now. I just wanna package it for me and the community.

  3. Done. That really did slipped through.

@06kellyjac
Copy link
Member

  1. ah true. One thing we could do is add nixfmt-rfc-style to the update script and run nixfmt on the deps file after its generated but that does also increase the length significantly from 256 to 1261!
    Ignore would be nice but doesn't seem to exist: Comment directive to disable nixfmt nixfmt#91
    ironically nixfmt-rfc-style when it updates will also have this problem when it updates as it needs some generated files: https://github.com/NixOS/nixpkgs/pull/336322/files
    cc: @NixOS/nix-formatting having some trouble and some help here would be appreciated 🙂
    Worst case we'll just need to ignore the red x each deps.nix update but if it blocks the merge-bot that'd be quite annoying.
  2. I was thinking # Is this still relevant in the fork?and # this too?
    if the comments and their related commented code could be either removed or added back in that'd be great
  3. 👍

@Scrumplex
Copy link
Member

The nixfmt action is only run on files that are new, or were already formatted using nixfmt before. So subsequent updates will not fail the nixfmt action because of deps.nix. The established way forward is to ignore the warning for autogenerated files until we have a more general solution to this.

@Kek5chen
Copy link
Contributor Author

Kek5chen commented Nov 22, 2024

I'll check the comments yea. Though it does build and run properly with everything working. But I'll make sure that these fixes don't impact anything anymore.

I've already discussed the formatting guidelines on Matrix. Matter of the fact is that they can be ignored since they are in development, and are still in progress / no definite guide has formed yet. The deps file can just stay as is, it shouldn't have an impact on anything.

@ofborg ofborg bot requested a review from 06kellyjac November 22, 2024 17:24
@ofborg ofborg bot added 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 22, 2024
@FliegendeWurst FliegendeWurst added the awaiting_changes (old Marvin label, do not use) label Dec 4, 2024
@Kek5chen Kek5chen removed the awaiting_changes (old Marvin label, do not use) label Dec 11, 2024
@Kek5chen
Copy link
Contributor Author

If someone could please confirm this works on wayland, that'd be nice.

@Kek5chen
Copy link
Contributor Author

oh and it'd be nice to know if it does build & run on x86_64-darwin

@github-actions github-actions bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1 labels Dec 11, 2024
@Gliczy
Copy link
Member

Gliczy commented Dec 11, 2024

Works fine with XWayland.

But it won't start if SDL_VIDEODRIVER is set to wayland (Ryujinx does not support Wayland).
You may consider forcing SDL_VIDEODRIVER to x11 (this is what the current ryujinx package do) to prevent that.

@Kek5chen
Copy link
Contributor Author

Alright then I'll just consider these two fixes to still be relevant.

@Redhawk18
Copy link
Contributor

Just from the looks of it, it looks like its almost done. Maybe inline the issues that are linked if anyone remembers what they said? But other than that it looks good.

@Redhawk18
Copy link
Contributor

just built and cloned, runs perfectly on wayland!

@artemist
Copy link
Member

Avalonia (the UI framework ryujinx uses) has a fork that supports Wayland, but upstream still doesn't. It will likely be a while until it's possible to use Wayland natively.

@Aleksanaa
Copy link
Member

@ofborg build ryujinx-greemdev

Copy link
Member

@Aleksanaa Aleksanaa left a comment

Choose a reason for hiding this comment

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

Eval failure is unrelated, formatting is unrelated, tested builds on aarch64-darwin

@Aleksanaa Aleksanaa merged commit 00aba09 into NixOS:master Dec 14, 2024
23 of 27 checks passed
@06kellyjac 06kellyjac mentioned this pull request Dec 16, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants