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

duckstation: 0.1-6292 -> 0.1-6658 #309064

Merged
merged 3 commits into from
May 29, 2024
Merged

duckstation: 0.1-6292 -> 0.1-6658 #309064

merged 3 commits into from
May 29, 2024

Conversation

AndersonTorres
Copy link
Member

@AndersonTorres AndersonTorres commented May 4, 2024 •
edited
Loading

Description of changes

Closes #308974 (by assimilation)

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.05 Release Notes (or backporting 23.05 and 23.11 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 May 4, 2024 16:20
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3878

@qubitnano
Copy link
Contributor

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

1 package built:
  • duckstation

Thank you

@ofborg ofborg bot requested a review from guibou May 4, 2024 16:49
@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 May 4, 2024
marcusramberg
marcusramberg approved these changes May 4, 2024
View reviewed changes
Copy link
Contributor

@marcusramberg marcusramberg left a comment

Choose a reason for hiding this comment

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

lgtm.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 4, 2024
@getchoo
Copy link
Member

getchoo commented May 5, 2024

Result of nixpkgs-review pr 309064 run on aarch64-linux 1

1 package built:
  • duckstation

@getchoo getchoo added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels May 5, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1618

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label May 5, 2024
SuperSandro2000
SuperSandro2000 requested changes May 6, 2024
View reviewed changes
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/by-name/du/duckstation/package.nix Show resolved Hide resolved
@SuperSamus SuperSamus mentioned this pull request May 7, 2024
Merged
13 tasks
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/call-for-discussion-why-and-how-much-should-we-care-about-the-override-interfaces-part-i-duckstation-and-pcsx2/45117/1

doronbehar
doronbehar reviewed May 15, 2024
View reviewed changes
pkgs/by-name/du/duckstation/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/du/duckstation/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/du/duckstation/package.nix Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
doronbehar
doronbehar reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Looks much better now!

pkgs/by-name/du/duckstation/package.nix Outdated Show resolved Hide resolved
@SuperSamus SuperSamus mentioned this pull request May 20, 2024
Closed
13 tasks
AndersonTorres
AndersonTorres commented May 22, 2024
View reviewed changes
Copy link
Member Author

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

.

doronbehar
doronbehar requested changes May 22, 2024
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Please minimize your diffs.

pkgs/by-name/du/duckstation/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/du/duckstation/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/du/duckstation/package.nix Outdated Show resolved Hide resolved
@AndersonTorres
Copy link
Member Author

Who added that "Check that Nix files are formatted" without telling it to everyone???

@github-actions github-actions bot added 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` and removed 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels May 22, 2024
@doronbehar
Copy link
Contributor

CI error:

/nix/store/xfhkjnpqjwlf6hlk1ysmq3aaq80f3bjj-stdenv-linux/setup: line 1581: out/share/duckstation/common-tests: No such file or directory

@doronbehar
Copy link
Contributor

Who added that "Check that Nix files are formatted" without telling it to everyone???

Where?

@AndersonTorres
Copy link
Member Author

Now it should work.

doronbehar
doronbehar requested changes May 24, 2024
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

I'm beginning to lose patience due to your insistence on enlarging the diff for no real good reason. I wanted to spare us the review-change ping pong, by rebasing your branch myself, but you are not allowing maintainers to edit the PR .

Your next trial is your last.

pkgs/by-name/du/duckstation/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/du/duckstation/package.nix Outdated Show resolved Hide resolved
@AndersonTorres
Copy link
Member Author

Anything else?

@AndersonTorres
Copy link
Member Author

but you are not allowing maintainers to edit the PR .

I am on that web interface of GitHub, and the "allow edits by maintainers" checkbox does not exist.
I don't know why.

I have opened another PR right now and can confirm: there is no checkbox.

edits

@AndersonTorres
Copy link
Member Author

Oh, anything else? [2]

@eclairevoyant
Copy link
Contributor

the "allow edits by maintainers" checkbox does not exist.

https://github.com/orgs/community/discussions/5634

Currently, if a PR is created from a fork living under an organisation (rather than an individual) it's not possible to allow edits by maintainers.

@AndersonTorres
Copy link
Member Author

@SuperSandro2000 and @doronbehar

Your both reviews are mutually incompatible.

Bringing back qt6.callPackage is not compatible to get rid of all-packages.nix.

I am unable to satisfy both of your reviews at the same time.

Please solve this between yourselves before I can take any action (besides rebasing this over Master Branch).

@AndersonTorres
Copy link
Member Author

And just for venting a decade-old frustration,
I have less than love for that less than beautiful dark-magic-accepted-by-blind-fanaticism thing called "splicing".

@AndersonTorres
duckstation: migrate to by-name
d9a0966 
Since by-name hierarchy does not support "third party" callPackage calls, the
expression was modified to get rid of qt6Packages.
@AndersonTorres
duckstation: checkPhase -> installCheckPhase
82a6593
@AndersonTorres
duckstation: 0.1-6292 -> 0.1-6658
9b1c668 
Duckstation now uses a vendored shaderc.
However, this vendoring is provided by a patch.
@infinisil
Copy link
Member

For easily finding it, a discussion is happening in this thread.

Bringing back qt6.callPackage is not compatible to get rid of all-packages.nix.

#309064 (comment) :)

Who added that "Check that Nix files are formatted" without telling it to everyone???

It's only for a small subset of files for now, people are very eager to use the standard formatter already :). For reference, #299578 introduced it, also #313628 is relevant. Also see #322520.

eclairevoyant
eclairevoyant approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

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

I didn't test properly running this in a game, but builds and starts fine.

@eclairevoyant eclairevoyant dismissed SuperSandro2000’s stale review May 29, 2024 00:15

splicing works without qt6.callPackage

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label May 29, 2024
@eclairevoyant eclairevoyant merged commit f5e912c into NixOS:master May 29, 2024
25 checks passed
@AndersonTorres AndersonTorres mentioned this pull request Jun 13, 2024
Merged
13 tasks
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/frustrations-about-splicing/49607/1

@doronbehar doronbehar mentioned this pull request Aug 4, 2024
Open
@AndersonTorres AndersonTorres mentioned this pull request Aug 12, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infinisil infinisil infinisil left review comments

@doronbehar doronbehar doronbehar approved these changes

@K900 K900 K900 left review comments

@marcusramberg marcusramberg marcusramberg approved these changes

@getchoo getchoo getchoo approved these changes

@eclairevoyant eclairevoyant eclairevoyant approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@guibou guibou Awaiting requested review from guibou

@SuperSamus SuperSamus Awaiting requested review from SuperSamus

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.