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.

Sign up for GitHub

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

Jump to bottom

pcsx2: rework #325562

Merged
merged 4 commits into from
Jul 11, 2024
Merged

pcsx2: rework #325562

merged 4 commits into from
Jul 11, 2024

Conversation

AndersonTorres
Copy link
Member

Description of changes

Merging two packages into one is not a good idea at all. Let's split it.

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.

@AndersonTorres AndersonTorres marked this pull request as ready for review July 8, 2024 15:15
@matteo-pacini
Copy link
Contributor

@AndersonTorres can we get this merged first, as it's been around for a while? 🙏🏻

#324113

@AndersonTorres AndersonTorres requested a review from afh July 8, 2024 15:21
superherointj
superherointj reviewed Jul 8, 2024
View reviewed changes
pkgs/by-name/pc/pcsx2/package.nix Outdated
repo = "pcsx2";
fetchSubmodules = true;
rev = "v${finalAttrs.version}";
sha256 = "sha256-WiwnP5yoBy8bRLUPuCZ7z4nhIzrY8P29KS5ZjErM/A4=";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sha256 = "sha256-WiwnP5yoBy8bRLUPuCZ7z4nhIzrY8P29KS5ZjErM/A4=";
hash = "sha256-WiwnP5yoBy8bRLUPuCZ7z4nhIzrY8P29KS5ZjErM/A4=";

@AndersonTorres
Copy link
Member Author

@AndersonTorres can we get this merged first, as it's been around for a while? 🙏🏻

#324113

Oh I can.

@AndersonTorres AndersonTorres marked this pull request as draft July 8, 2024 15:29
@afh
Copy link
Member

afh commented Jul 8, 2024

Thanks for pinging me on this one, @AndersonTorres. Possibly my understanding of a package differs from what nixpkgs considers a package. In my mind a package is a piece of software that can be installed, regardless of whether from source or from binary.

This PR seems to suggest that nixpkgs differentiates between source builds and binary installs. Can you please point me to documentation that outlines the reasons why such a distinction is made? I'm genuinely curious 🙂

@superherointj
Copy link
Contributor

superherointj commented Jul 8, 2024 •
edited
Loading

Thanks for pinging me on this one, @AndersonTorres. Possibly my understanding of a package differs from what nixpkgs considers a package. In my mind a package is a piece of software that can be installed, regardless of whether from source or from binary.

This PR seems to suggest that nixpkgs differentiates between source builds and binary installs. Can you please point me to documentation that outlines the reasons why such a distinction is made? I'm genuinely curious 🙂

I believe, the matter is how packages are referenced by-name, not some conceptual notion of package.

https://github.com/NixOS/rfcs/blob/master/rfcs/0140-simple-package-paths.md

Two top-level entries, two directories. Is that the issue?

@AndersonTorres
Copy link
Member Author

@afh

In my mind a package is a piece of software that can be installed, regardless of whether from source or from binary.

Yes, let's pick this definition.

This PR seems to suggest that nixpkgs differentiates between source builds and binary installs.

The difference is more pronounced. This is a similar situation from vlc vs vlc-bin:

Both packages follow completely distinct methods of deployment:

One is binary-only, MacOS-only, can't be customized and merely repacked.
The other is effectively deployed from sources, and albeit being Linux-only, it can possibly be customized.

Further, it creates an unreasonable coupling.
Suppose the next release of PCSX2 uses new libraries.
The precompiled file can be released almost immediately, while the script to build the source one needs to be revised and updated.
It makes no sense to couple both releases to the same script.

@afh afh mentioned this pull request Jul 9, 2024
Open
13 tasks
@AndersonTorres
Copy link
Member Author

Now we have a "small" problem: Darwin users will need to call pcsx2-bin instead of merely pcsx.

If this is such a big issue, I can rename pcsx2 to pcsx2-src and green-alias both according to the platform.

@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package labels Jul 10, 2024
@ofborg ofborg bot requested review from GovanifY and hrdinka July 10, 2024 04:23
@afh
Copy link
Member

afh commented Jul 10, 2024

Possibly I'm misguided by my own misconceptions of nixpkgs or general preconceptions or even both 😅

First and foremost as written words can easily misread in their tone:
The mindset in which the following was written was friendly, appreciative, cooperative, and constructive. If it is perceived it otherwise for whatever reasons I kindly ask you, esteemed reader, to re-read this comment in the intended spirit and would appreciate feedback where the wording was misunderstood or ill-chosen 🙂

I think separating packages by platform or installation method, i.e. from source or pre-built binary is heading into the right direction, yet there are several things that don't feel right to me about this as it is currently.

  1. I find that a -bin or -src suffix makes discovering the package harder than it needs to be.
  2. The -bin or -src suffix in this PR seem to be bound to the platform that the package is being installed on. What if in the future nixpkgs would like to offer a upstream-built binary for linux of pcsx2? Where would that reside in the nixpkgs (repo) tree?
  3. As a user of a platform—in this case darwin—it does not matter to me whether the package is going to be installed from source or a pre-built binary and I would find it confusing if the package were named differently, in this case pcsx2-bin especially if the reason for this is a technical issue, possibly even a limitation of the current state of nixpkgs.
  4. As a maintainer or contributor I'd prefer all related Nix code to reside under a single directory (or as much as possible)

I'm very much in favor of the pkgs/by-name direction that nixpkgs is heading in (especially with NixOS/rfc#146 and #325357 being actively worked on 👏) yet I do see value in a os-specific tree. Would it be possible and improve this PR by git mv pkgs/by-name/pc/pcsx2-bin/package.nix pkgs/by-name/pc/pcsx2/darwin.nix and adding pcsx2 = ../pkgs/by-name/pc/pcsx2/darwin.nix { }; to pkgs/top-level/darwin-packages.nix?

Another approach could be to pcsx2 = pkgs.pcsx2-bin; in pkgs/top-level/darwin-aliases.nix and somehow inform darwin users who are trying to install pcsx2 that the package is available on their platforms as darwin.pcsx2 and/or pcsx2-bin.

I don't think I properly understand what you mean by "green-alias", @AndersonTorres, could you provide more context and an example, please?

@AndersonTorres
Copy link
Member Author

I think separating packages by platform or installation method, i.e. from source or pre-built binary is heading into the right direction, yet there are several things that don't feel right to me about this as it is currently.

1. I find that a `-bin` or `-src` suffix makes discovering the package harder than it needs to be.

The idea is to write something like

pcsx2 = if stdenv.isDarwin then pcsx2-bin else pcsx2-src;
2. The `-bin` or `-src` suffix in this PR seem to be bound to the platform that the package is being installed on.

Not exactly. The -bin is bound to the platform by its own nature, but -src is bound because we didn't figure out how to build it for both.

What if in the future nixpkgs would like to offer a upstream-built binary for linux of pcsx2?

In Nixpkgs we prefer source-based builds. The upstream binary package would be rejected.
Indeed we are only including the MacOS binary package because we didn't figure how to build it from sources yet.

The only exception I can imagine is when the binary package has some closed-source extras that are not included by default on the sources.

I don't think I properly understand what you mean by "green-alias", @AndersonTorres, could you provide more context and an example, please?

By design, aliases put in pkgs/top-level/aliases.nix are rejected by OfBorg, in the following sense: using pkgconfig in your code is fine, but when uploading it to Nixpkgs, ofBorg selectively ignores aliases.nix and do not recognize pkgconfig as an alias for pkg-config.

A "green alias" would be one put in all-packages.nix.

@AndersonTorres AndersonTorres marked this pull request as ready for review July 10, 2024 13:35
@afh
Copy link
Member

afh commented Jul 10, 2024

I'm very much for the idea to:

pcsx2 = if stdenv.isDarwin then pcsx2-bin else pcsx2-src;

I think this reflects the state of things very well 👌 and look forward to the necessary changes for this in this PR 🙂

In Nixpkgs we prefer source-based.

That makes sense, after all reproducibility is an important aspect of Nix and nixpkgs.

Thanks for the explanation of green aliases, that's helpful to know.

matteo-pacini
matteo-pacini approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@matteo-pacini matteo-pacini left a comment

Choose a reason for hiding this comment

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

LGTM - I'm happy with pcsx2-bin for Darwin, as it explicitly tells me it's a binary distribution.

@ofborg ofborg bot requested a review from matteo-pacini July 11, 2024 00:04
@AndersonTorres
pcsx2: "remove" macos binary package
9c6d94f 
Let's "remove" the binary-only, MacOS-only package from this expression, because
it should be maintained by MacOS maintainers.

Besides, no other modification will be made right now, just a merge between files.
@AndersonTorres
pcsx2: fix expression
eb7ef34 
- lacks runHooks on installPhase
- remove nested with
@AndersonTorres
pcsx2: big rewrite
f277070 
- Gather all the sources in a same expression under sources.nix
- Doc-comment and rename the patch
- strictDeps is set as true
  - Why extra-cmake-modules does not work in nativeBuildInputs??
- Add myself as maintainer
@AndersonTorres
pcsx2-bin: "init" at 1.7.5919
a47fc28 
This is a repack of MacOS-only precompiled package from upstream.

Basically it is a migration of the previous Darwin-only package for pcsx2.

By request, matteo-pacini is set as the sole maintainer of this package.
@superherointj
Copy link
Contributor

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

1 package built:
  • pcsx2

@superherointj
Copy link
Contributor

Result of nixpkgs-review pr 325562 run on aarch64-darwin 1

@superherointj superherointj merged commit 7b3c572 into NixOS:master Jul 11, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@superherointj superherointj superherointj approved these changes

@afh afh Awaiting requested review from afh

@GovanifY GovanifY Awaiting requested review from GovanifY

@hrdinka hrdinka Awaiting requested review from hrdinka

@matteo-pacini matteo-pacini Awaiting requested review from matteo-pacini

Assignees
No one assigned
Labels
8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AndersonTorres @matteo-pacini @afh @superherointj