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

libretro.citra: fix broken build (ffmpeg -> ffmpeg_6) #352220

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

Frontear
Copy link
Member

Closes #351162

Things done

  • 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.

@Frontear
Copy link
Member Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 352220


x86_64-linux

✅ 2 packages built:
  • libretro.citra
  • retroarchFull

@khaneliman
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 352220


x86_64-linux

✅ 2 packages built:
  • libretro.citra
  • retroarchFull

aarch64-linux

✅ 2 packages built:
  • libretro.citra
  • retroarchFull

x86_64-darwin

⏩ 1 package marked as broken and skipped:
  • libretro.citra

aarch64-darwin

⏩ 1 package marked as broken and skipped:
  • libretro.citra

Copy link
Contributor

@khaneliman khaneliman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thiagokokada
Copy link
Contributor

CC @emilazy since you're trying to replace the older versions of ffmpeg.

@thiagokokada thiagokokada merged commit 9e88ad8 into NixOS:master Oct 30, 2024
29 of 30 checks passed
@emilazy
Copy link
Member

emilazy commented Oct 30, 2024

Upstream Citra is a dead project so I worry somewhat that this will never be fixed. Actually, I’m somewhat surprised that we carry this fork that has been basically untouched all year and is behind where upstream Citra was when we dropped it in #293312.

An active‐until‐yesterday(!) fork of Citra, used by things like the AUR package, merged an FFmpeg 7 compatibility PR: PabloMK7/citra#248. If we’re keeping this package but not actual Citra – though it seems a little odd to me given that development is more inactive than Citra or its forks – then we should probably apply that PR instead. I would prefer vendoring it in that case, given that the repository is now archived.

@thiagokokada
Copy link
Contributor

Upstream Citra is a dead project so I worry somewhat that this will never be fixed. Actually, I’m somewhat surprised that we carry this fork that has been basically untouched all year and is behind where upstream Citra was when we dropped it in #293312.

An active‐until‐yesterday(!) fork of Citra, used by things like the AUR package, merged an FFmpeg 7 compatibility PR: PabloMK7/citra#248. If we’re keeping this package but not actual Citra – though it seems a little odd to me given that development is more inactive than Citra or its forks – then we should probably apply that PR instead. I would prefer vendoring it in that case, given that the repository is now archived.

I have no strong feelings for us to switch to this fork. The only reason I think we never switch is because nobody bothered to open a PR.

@emilazy
Copy link
Member

emilazy commented Oct 30, 2024

I am not sure we can switch to it, since this is a libretro fork and I don’t know that Citra ever supported libretro itself. We removed Citra proper and this libretro version is behind upstream Citra, so it surprises me that we’re choosing to still carry this, but if we do then we should try to apply the FFmpeg 7 PR from the Citra fork I linked.

@thiagokokada
Copy link
Contributor

I am not sure we can switch to it, since this is a libretro fork and I don’t know that Citra ever supported libretro itself. We removed Citra proper and this libretro version is behind upstream Citra, so it surprises me that we’re choosing to still carry this, but if we do then we should try to apply the FFmpeg 7 PR from the Citra fork I linked.

Oh ok, I thought this fork supported libretro.

In this case, I am fine either using the ffmpeg embedded in libretro/citra itself or backporting the ffmpeg patches if possible.

@Frontear
Copy link
Member Author

but if we do then we should try to apply the FFmpeg 7 PR from the Citra fork I linked.

I'll do a little bit of investigating and see if I an propose a more elegant solution to this derivation, either by updating source or patching!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build failure: retroarchFull because of libretro citra core
4 participants